Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure possible padding bytes are initialized #1002

Closed
wants to merge 1 commit into from

Conversation

qarni
Copy link

@qarni qarni commented Jul 31, 2024

Microsoft's static analysis tool found a vulnerability from possible info leakage from uninitialized padding bytes in this struct, which I changed in microsoft/cmake. Pushing this change upstream so it can be added to kitware/cmake.

@madler
Copy link
Owner

madler commented Aug 1, 2024

Please provide the output of the static analysis tool. Thanks.

@qarni
Copy link
Author

qarni commented Aug 1, 2024

Utilities/cmzlib/gzlib.c#L113 Memory allocation of [struct <unnamed>] /Utilities/cmzlib/gzguts.h#L170 includes uninitialized padding bytes. 

Possible information leakage from uninitialized padding bytes.
A newly allocated struct or class that is initialized member-by-member may leak information if it includes padding bytes.
Recommendation: Make sure that all padding bytes in the struct or class are initialized.

If possible, use memset to initialize the whole structure/class.

(Line numbers are a little off since kitware/cmake hasn't pulled in the newest version of zlib in a bit)

@madler
Copy link
Owner

madler commented Aug 1, 2024

Thanks. What version of zlib was this run on?

@qarni
Copy link
Author

qarni commented Aug 1, 2024

zlib 1.2.13

@madler
Copy link
Owner

madler commented Aug 1, 2024

According to this document: https://wiki.sei.cmu.edu/confluence/display/c/DCL39-C.+Avoid+information+leakage+when+passing+a+structure+across+a+trust+boundary , Microsoft's suggested remedy of using memset() does not in fact solve the potential security issue, shown there as a "noncompliant code example".

In any case, the issue is an issue only if the structure is being passed across a trust boundary, e.g., out of the kernel. zlib does no such passing. The structure in question is internal to zlib and not accessible to the application through the zlib interface. zlib provides no facility to copy a gzFile object. In order for the application to copy a gzFile object, to get it across one of these trust boundaries, it would need to use gzguts.h to even know the size of the internal structure. Once an application is getting into zlib's pants, then all bets are off, and it is up to the application to guard against any security issues it may create. In that case, the correct remedy in the linked document is to copy the structure element-by-element. Since the application is already using gzguts.h, it has the information it needs to do that.

Thank you for your report. Nothing for zlib here. I am closing this issue.

@madler madler closed this Aug 1, 2024
@Neustradamus Neustradamus mentioned this pull request Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants