NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.3k stars 13.54k forks source link

glibc bump causes reproducible buffer overflow in privoxy with minimal socks4 config #265654

Closed iblech closed 7 months ago

iblech commented 10 months ago

Describe the bug

Even with a minimal one-line config, privoxy runs into a buffer overflow on trying to serve a request. This only happens when privoxy is instructed to use a socks4 or socks4a upstream proxy, not with socks5 or socks5t. This is on current nixpkgs-unstable.

I confirmed that this bug does not occur with the old commit 2f4ea5fab6dda0199cb5e65d85b5af2eed4de14d of nixpkgs, the last time our privoxy build was touched (version-bumped). So the version bump of privoxy it not at fault.

$ nix-shell -p privoxy --run "privoxy --no-daemon <(echo 'forward-socks4 / 127.0.0.1:9050 .')"
2023-11-05 14:05:09.531 7fef77483740 Info: Privoxy version 3.0.34
2023-11-05 14:05:09.531 7fef77483740 Info: Program name: privoxy
*** buffer overflow detected ***: terminated
/tmp/nix-shell-320481-0/rc: line 3: 321835 Aborted                 (core dumped) privoxy --no-daemon <(echo 'forward-socks4 / 127.0.0.1:9050 .')

Steps To Reproduce

Steps to reproduce the behavior:

  1. Run nix-shell -p privoxy --run "privoxy --no-daemon <(echo 'forward-socks4 / 127.0.0.1:9050 .')" in the background (even if you don't have a socks proxy listening on 127.0.0.1:9050)
  2. Run http_proxy=http://localhost:8118 curl http://google.com/
  3. Observe that the privoxy process aborts

Expected behavior

Privoxy should not segfault, instead it should properly serve the request (or display an error in case upstream proxy servers were not reachable).

Notify maintainers

No maintainers listed.

Metadata

[user@system:~]$ nix-shell -p nix-info --run "nix-info -m"
 - system: `"x86_64-linux"`
 - host os: `Linux 6.5.9, NixOS, 23.11 (Tapir), 23.11pre544052.85f1ba3e5167`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.17.1`
 - channels(root): `"nixos"`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`
iblech commented 10 months ago

I'm currently bisecting. My computer is busy compiling glibc, hence definitive results will still take some time. But I'm down to 9 revisions to test (roughly 3 steps).

Pinging @Ma27 because all of those remaining 9 revisions pertain to glibc.

Update: Found the commit introducing this bug, it's e86152986c6e6c563ced037c91403318d5a8b447, which bumped glibc from 2.37-39 to 2.38-0.

git bisect log ``` # status: waiting for both good and bad commits # bad: [5eda4619591bc659942cd03d1bb3b04925bb1435] Merge pull request #265605 from r-ryantm/auto-update/python310Packages.cx-freeze git bisect bad 5eda4619591bc659942cd03d1bb3b04925bb1435 # status: waiting for good commit(s), bad commit known # bad: [d80d7d66977956d13ae476b898d6b93532f3636f] Merge pull request #258071 from helsinki-systems/feat/stc-lock git bisect bad d80d7d66977956d13ae476b898d6b93532f3636f # status: waiting for good commit(s), bad commit known # good: [6b20f99fc7b7b7b8f38989537177c4908bfef5e7] Merge pull request #262448 from khaneliman/lazydocker git bisect good 6b20f99fc7b7b7b8f38989537177c4908bfef5e7 # bad: [8ecf5abf794ab364b739df456bbcec41f7fa95aa] Merge pull request #262388 from drupol/php/build-support/make-validation-non-blocking git bisect bad 8ecf5abf794ab364b739df456bbcec41f7fa95aa # bad: [755ef90835c768bdb18aff9f6c96ccbb8667b290] mailmanPackages: pin redis to 4.5.4 git bisect bad 755ef90835c768bdb18aff9f6c96ccbb8667b290 # good: [b0279b8daa17ef145b5442d295b4d920d782b7c0] python3Packages.pycodestyle: 2.10.0 -> 2.11.0 git bisect good b0279b8daa17ef145b5442d295b4d920d782b7c0 # good: [580cb8df0a9308ae89368fb3ebfcffb33b954f12] python311Packages.dirty-equals: run tests in passthru.tests git bisect good 580cb8df0a9308ae89368fb3ebfcffb33b954f12 # bad: [e107233b2a7c11efc85613d949d341d392d2a500] Merge #257727: gst_all_1.*: 1.22.5 -> 1.22.6 git bisect bad e107233b2a7c11efc85613d949d341d392d2a500 # good: [2707ba5a632ad3727d283925c179ce3defd06561] python311Packages.ppft: add changelog to meta git bisect good 2707ba5a632ad3727d283925c179ce3defd06561 # good: [bfaeb8bbc6a5553098bab7cb56ef3f1b1709c559] python311Packages.tables: fix test for numexpr 2.8.5 git bisect good bfaeb8bbc6a5553098bab7cb56ef3f1b1709c559 # bad: [f7e03f48514acdcb5491c87afbcaefebc86b6c70] Merge pull request #251878 from NixOS/python-updates git bisect bad f7e03f48514acdcb5491c87afbcaefebc86b6c70 # bad: [8348b18c096d60e5b5821157f7e8df781330fed4] glibc: 2.38-0 -> 2.38-23 git bisect bad 8348b18c096d60e5b5821157f7e8df781330fed4 # bad: [774a808ec9336326107de4096a07c22ed9c95a8e] kvmtool: fix build w/ glibc-2.38 git bisect bad 774a808ec9336326107de4096a07c22ed9c95a8e # bad: [d00a220853e808e2fcc9b951628645e779d56db6] direwolf: fix build w/ glibc-2.38 git bisect bad d00a220853e808e2fcc9b951628645e779d56db6 # bad: [e86152986c6e6c563ced037c91403318d5a8b447] glibc: 2.37-39 -> 2.38-0 git bisect bad e86152986c6e6c563ced037c91403318d5a8b447 # first bad commit: [e86152986c6e6c563ced037c91403318d5a8b447] glibc: 2.37-39 -> 2.38-0 ```
gdb backtrace ``` (gdb) bt #0 0x00007ffff77f7d7c in __pthread_kill_implementation () from /nix/store/gqghjch4p1s69sv4mcjksb2kb65rwqjy-glibc-2.38-23/lib/libc.so.6 #1 0x00007ffff77a89c6 in raise () from /nix/store/gqghjch4p1s69sv4mcjksb2kb65rwqjy-glibc-2.38-23/lib/libc.so.6 #2 0x00007ffff77918fa in abort () from /nix/store/gqghjch4p1s69sv4mcjksb2kb65rwqjy-glibc-2.38-23/lib/libc.so.6 #3 0x00007ffff7792767 in __libc_message.cold () from /nix/store/gqghjch4p1s69sv4mcjksb2kb65rwqjy-glibc-2.38-23/lib/libc.so.6 #4 0x00007ffff7886749 in __fortify_fail () from /nix/store/gqghjch4p1s69sv4mcjksb2kb65rwqjy-glibc-2.38-23/lib/libc.so.6 #5 0x00007ffff7886104 in __chk_fail () from /nix/store/gqghjch4p1s69sv4mcjksb2kb65rwqjy-glibc-2.38-23/lib/libc.so.6 #6 0x00007ffff7887b59 in __strlcpy_chk () from /nix/store/gqghjch4p1s69sv4mcjksb2kb65rwqjy-glibc-2.38-23/lib/libc.so.6 #7 0x0000555555579638 in socks4_connect () #8 0x000055555557aac4 in forwarded_connect () #9 0x00005555555802a2 in chat () #10 0x0000555555581905 in serve () #11 0x00007ffff77f6084 in start_thread () from /nix/store/gqghjch4p1s69sv4mcjksb2kb65rwqjy-glibc-2.38-23/lib/libc.so.6 #12 0x00007ffff787855c in clone3 () from /nix/store/gqghjch4p1s69sv4mcjksb2kb65rwqjy-glibc-2.38-23/lib/libc.so.6 ```
esclear commented 10 months ago

Reading through the release announcement for 2.38-0, I found the following:

A new configure option, "--enable-fortify-source", can be used to build the GNU C Library with _FORTIFY_SOURCE. The level of fortification can either be provided, or is set to the highest value supported by the compiler. If not explicitly enabled, then fortify source is forcibly disabled so to keep original behavior unchanged.

And e861529 enabled source fortification. It seems likely to me that this may be an issue in privoxy, which is now caught by the new glibc.

Edit: Ah, your backtrace seems to confirm that: #4 0x00007ffff7886749 in __fortify_fail ()

FliegendeWurst commented 10 months ago

This might be related to #262080. (However, that issue was bisected to glibc: 2.37-8 -> 2.37-39)

iblech commented 10 months ago

Thank you, that makes sense! I'm currently believing that the issue is caused by this call to strlcpy() in the Privoxy source. But I didn't verify this hypothesis yet and need to run now.

Update: That's confirmed. Removing this call gets rid of the fortify-caused abort. The compiler warning is also interesting:

gateway.c: In function 'socks4_connect':
gateway.c:840:4: warning: 'strlcpy' writing 4988 bytes into a region of size 1 overflows the destination [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-overflow=-Wstringop-overflow=8;;]
  840 |    strlcpy(&(c->userid), socks_userid, sizeof(buf) - sizeof(struct socks_op));
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gateway.c:112:9: note: destination object 'userid' of size 1
  112 |    char userid;               /* first byte of userid */
      |         ^~~~~~

On inspection, the buffer is actually large enough, but this fact is not properly detected.

Update: I intend to change &(c->userid) into buf + offsetof(struct socks_op, userid) (in line 840 of the linked gateway.c). This convinces gcc that there is enough space for the strlcpy() operation. It should semantically be the same (note that c and buf point to the same location, they just differ in their types). I welcome comments regarding this proposed fix, will then take care of upstream reporting and submitting an interim pull request.