5 ms·

Re: Zlib memory corruption on deflate

53 points by 0x0 5y ago

rurban 5y ago
what is more concerning is that the maintainer didn't understand this impact case, even when two people showed him the exact logic, linenumber and a reproducer. he was still the opinion that this cannot happen in production. we need a zlib fork
- mkj 5y ago
  memLevel=1 afaik is pretty rare in the wild? The maintainer doesn't seem to dispute it after a reproducer was shown, a new release is out now.
  - adzm 5y ago
    default memLevel is 8. Memory usage is basically defined by windowBits and memLevel; 1 would be really unusual, even for embedded scenarios.
    - plorkyeran 5y ago
      Yeah, the existence of memLevel is basically a historical quirk. The total memory used by zlib at default memLevel and windowBits is under 300KB (it needs one 256KB buffer and a few small additional allocations). There's not a lot of environments left where it makes sense to run zlib where that's too much.
  - rurban 5y ago
    I showed him the exact problematic line, but he didn't seem to understand it. same thing happened previously.
- jsnell 5y ago
  There are already active zlib forks (e.g. https://github.com/zlib-ng/zlib-ng https://github.com/zlib-ng/zlib-ng), the problem is with having people move to them. It takes a lot of effort to move mindshare from the original version to a fork, there's some historical examples of it happening, but not a ton.
  - rurban 5y ago
    ah, thanks. this looks indeed as not from the last century. much better code.
mdb31 5y ago
This seems to be a minor bug, in that it is only triggered using unusual (and rather unlikely) deflate parameters. But, looking into this bug, I was sort-of interested to see how it was handled. The change log for 1.2.12 (2022-03-27) indicates the issue was resolved ("Fix a bug that can crash deflate on some input when using Z_FIXED"). Yet, in what seems to be the canonical Zlib repository (https://github.com/madler/zlib https://github.com/madler/zlib), I was unable to find a corresponding commit. None of commits this year (7, so not too hard to review) seem to be particularity meaningful changes, and in particular yesterday's 'zlib 1.2.12' commit seems to only consist of version/copyright updates. So, does anyone have any idea where to find the commits related to the change log entry? (Note that I'm not disputing the issue is actually fixed, I'm just trying to improve my Github reading skills...)
- kam 5y ago
  It was fixed in 2018, but no release was made at the time https://github.com/madler/zlib/commit/5c44459c3b28a9bd3283aaceab7c615f8020c531 https://github.com/madler/zlib/commit/5c44459c3b28a9bd3283aa...
  - mdb31 5y ago
    Thank you very much! Sort-of surprising there is so much discussion about a long-fixed bug, yet, there we are...
    - pajko 5y ago
      A lot of older projects (especially statically linked Windows applications) might contain an older release of zlib which is still affected by the bug. Don't know anything about the nature of the bug, but if the corruption could be converted to memory corruption... we have a problem.
  - benibela 5y ago
    I wonder if FreePascal is affected Looks like they ported zlib to Pascal in 1998 and left it pretty much unchanged: https://gitlab.com/freepascal.org/fpc/source/-/blob/main/packages/paszlib/src/zdeflate.pas#L580 https://gitlab.com/freepascal.org/fpc/source/-/blob/main/pac...
MauranKilom 5y ago
For lack of better avenues, I'll repeat this here: Don't use the hand-optimized assembly code that comes with zlib. It will read out of bounds given the right input data. By default, this affects the VS solutions, but please double-check your build. More info: https://news.ycombinator.com/item?id=30084771 https://news.ycombinator.com/item?id=30084771

53 points by 0x0 5y ago

what is more concerning is that the maintainer didn't understand this impact case, even when two people showed him the exact logic, linenumber and a reproducer. he was still the opinion that this cannot happen in production. we need a zlib fork

mkj 5y ago
memLevel=1 afaik is pretty rare in the wild? The maintainer doesn't seem to dispute it after a reproducer was shown, a new release is out now.
- adzm 5y ago
  default memLevel is 8. Memory usage is basically defined by windowBits and memLevel; 1 would be really unusual, even for embedded scenarios.
  - plorkyeran 5y ago
    Yeah, the existence of memLevel is basically a historical quirk. The total memory used by zlib at default memLevel and windowBits is under 300KB (it needs one 256KB buffer and a few small additional allocations). There's not a lot of environments left where it makes sense to run zlib where that's too much.
- rurban 5y ago
  I showed him the exact problematic line, but he didn't seem to understand it. same thing happened previously.
jsnell 5y ago
There are already active zlib forks (e.g. https://github.com/zlib-ng/zlib-ng https://github.com/zlib-ng/zlib-ng), the problem is with having people move to them. It takes a lot of effort to move mindshare from the original version to a fork, there's some historical examples of it happening, but not a ton.
- rurban 5y ago
  ah, thanks. this looks indeed as not from the last century. much better code.

mdb31 5y ago

This seems to be a minor bug, in that it is only triggered using unusual (and rather unlikely) deflate parameters. But, looking into this bug, I was sort-of interested to see how it was handled. The change log for 1.2.12 (2022-03-27) indicates the issue was resolved ("Fix a bug that can crash deflate on some input when using Z_FIXED"). Yet, in what seems to be the canonical Zlib repository (https://github.com/madler/zlib https://github.com/madler/zlib), I was unable to find a corresponding commit. None of commits this year (7, so not too hard to review) seem to be particularity meaningful changes, and in particular yesterday's 'zlib 1.2.12' commit seems to only consist of version/copyright updates. So, does anyone have any idea where to find the commits related to the change log entry? (Note that I'm not disputing the issue is actually fixed, I'm just trying to improve my Github reading skills...)

kam 5y ago
It was fixed in 2018, but no release was made at the time https://github.com/madler/zlib/commit/5c44459c3b28a9bd3283aaceab7c615f8020c531 https://github.com/madler/zlib/commit/5c44459c3b28a9bd3283aa...
- mdb31 5y ago
  Thank you very much! Sort-of surprising there is so much discussion about a long-fixed bug, yet, there we are...
  - pajko 5y ago
    A lot of older projects (especially statically linked Windows applications) might contain an older release of zlib which is still affected by the bug. Don't know anything about the nature of the bug, but if the corruption could be converted to memory corruption... we have a problem.
- benibela 5y ago
  I wonder if FreePascal is affected Looks like they ported zlib to Pascal in 1998 and left it pretty much unchanged: https://gitlab.com/freepascal.org/fpc/source/-/blob/main/packages/paszlib/src/zdeflate.pas#L580 https://gitlab.com/freepascal.org/fpc/source/-/blob/main/pac...

MauranKilom 5y ago

For lack of better avenues, I'll repeat this here: Don't use the hand-optimized assembly code that comes with zlib. It will read out of bounds given the right input data. By default, this affects the VS solutions, but please double-check your build. More info: https://news.ycombinator.com/item?id=30084771 https://news.ycombinator.com/item?id=30084771