curl / trurl

trurl is a command line tool for URL parsing and manipulation.
https://curl.se/trurl/
Other
3.1k stars 99 forks source link

Fix buffer overflows when accessing data allocated in memdupdec #270

Closed jacobmealey closed 6 months ago

jacobmealey commented 6 months ago

This is a culmination of work to address memory errors found by @fusl in https://github.com/curl/trurl/issues/265, https://github.com/curl/trurl/issues/266 and https://github.com/curl/trurl/issues/267. Unsure if we need to add explicit tests for the cases used for testing -- mostly because I'm not sure how trurl should handle these weird urls, but we can include them for the sake of memory testing if people think its needed.

Fixes #265 Fixes #266 Fixes #267

bagder commented 6 months ago

Unsure if we need to add explicit tests for the cases used for testing -- mostly because I'm not sure how trurl should handle these weird urls, but we can include them for the sake of memory testing if people think its needed.

I think we should. For the memory tests, but also to just make sure that we make a decision on how to act on them and make sure we think trurl behaves correctly.

bagder commented 6 months ago

I won't mind to merge this PR first though and we can work on adding more test cases based on this in a separate PR. Let me know what you prefer.

jacobmealey commented 6 months ago

I won't mind to merge this PR first though and we can work on adding more test cases based on this in a separate PR. Let me know what you prefer.

I think that makes sense - I was mostly looking at these two instances.

$ ./trurl "0?00%000000000000000000000=0000000000"
http://0.0.0.0/?00%000000000000000000000=0000000000
$ ./trurl --json 0?0%000000000000000000000000000000000 
[
  {
    "url": "http://0.0.0.0/?0%000000000000000000000000000000000",
    "parts": {
      "scheme": "http",
      "host": "0.0.0.0",
      "path": "/"
    },
    "params": [
      {
        "key": "0\u00000000000000000000000000000000000",
        "value": ""
      }
    ]
  }
]

specifically the null encoding in the json seems wrong. I would expect it look to like 0\u0000\u0000\u000.... Is it that the first four zeros after the \u are part of the null encoding and all the following 0 are just regular ascii characters?

I can open a discussion or something about it this evenin

bagder commented 6 months ago

Is it that the first four zeros after the \u are part of the null encoding and all the following 0 are just regular ascii characters?

I'm certainly not a json wizard, but yes that is my impression. The \u is followed by four digits to form a unicode character, then follows regular ascii digits.

jacobmealey commented 6 months ago

Okay, perfect. I added the tests so this should be all good to go.

Something I want to add soon is an action that uses debug symbols from curl, because valgrind and the address sanitizer don't seem to keep track of memory allocations from release versions of libcurl so there have been a few sneaky memory leaks and stuff that have gone unnoticed - though I've addressed as many as I can find.

Edit: stupid rebase issues. resolving now.

jacobmealey commented 6 months ago

Okay, rebase conflicts are all resolved, this should now be good to pull in

bagder commented 6 months ago

Thanks!