-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
*** buffer overflow detected *** with GCC 12 and -D_FORTIFY_SOURCE=3 #22801
Comments
I am not sure I follow, but aren't you just saying that _FORTIFY_SOURCE=3 is just broken, and disagrees with If so, why is this a systemd bug and not a gcc bug? |
Yeah, it's a grey area and I bet Siddhesh can take a look at it. Just reading the
So the question is if you really need the functionality as it's intended for debugging or introspection purposes? |
it makes life a lot more pleasant to use this. And we use it all over the place, for example for things like strextend() and stuff. There were some other checkers like this in the past that had similar issues like this. But we got reassurances from libc people that malloc_usable_size() means what it says, and our usage is safe: |
Not all malloc implementations implement However, given that |
That said, will you accept @poettering a pull request where we'll use |
why would we ever want to use __builtin_object_size() if __builtin_dynamic_object_size() is available here? i.e. shouldn't we just switch unconditionally if the new builtin exists? why conditoinalize on _FORTIFY_SOURCE at all regarding its use? |
Yes, I would do that. Can you please make such a change? |
As explained in the issue, -D_FORTIFY_SOURCE=3 requires usage of __builtin_dynamic_object_size in MALLOC_SIZEOF_SAFE macro. Fixes: systemd#22801
As explained in the issue, -D_FORTIFY_SOURCE=3 requires usage of __builtin_dynamic_object_size in MALLOC_SIZEOF_SAFE macro. Fixes: systemd#22801
As explained in the issue, -D_FORTIFY_SOURCE=3 requires usage of __builtin_dynamic_object_size in MALLOC_SIZEOF_SAFE macro. Fixes: #22801
Notably not bothering to revbump for now because this manifests during self-execution during build and FORTIFY_SOURCE=3 is only available in GCC 12 which isn't even released yet, let alone exposed or enabled by default in Gentoo. It's far more likely that systemd 251 will be released (or at least another RC for it) before we're even close to unleashing FORTIFY_SOURCE=3 on Gentoo Hardened users by default. Bug: systemd/systemd#22801 Signed-off-by: Sam James <sam@gentoo.org>
I've just opened #23621 where I reverted this patch. I'm not sure how to make |
…_size." This reverts commit 0bd2925. Unlike __builtin_object_size, __builtin_dynamic_object_size is called at runtime and it isn't guaranteed anywhere that it always works with every pointer passed to it. It currently works with gcc because it returns -1 most of the time (which means that malloc_usable_size is used more often than not) but with clang (and probably gcc in the foreseeable future) it's just not safe to assume that all pointers can be handled at runtime. Closes systemd#23619 and systemd#23150. Reopens systemd#22801
…_size." This reverts commit 0bd2925. Unlike __builtin_object_size, __builtin_dynamic_object_size is called at runtime and it isn't guaranteed anywhere that it always works with every pointer passed to it. It currently works with gcc because it returns -1 most of the time (which means that malloc_usable_size is used more often than not) but with clang (and probably gcc in the foreseeable future) it's just not safe to assume that all pointers can be handled at runtime. Closes systemd#23619 and systemd#23150. Reopens systemd#22801
…_size." This reverts commit 0bd2925. Unlike __builtin_object_size, __builtin_dynamic_object_size is called at runtime and it isn't guaranteed anywhere that it always works with every pointer passed to it. It currently works with gcc because it returns -1 most of the time (which means that malloc_usable_size is used more often than not) but with clang (and probably gcc in the foreseeable future) it's just not safe to assume that all pointers can be handled at runtime. Closes systemd#23619 and systemd#23150. Reopens systemd#22801
…_size." This reverts commit 0bd2925. It isn't guaranteed anywhere that __builtin_dynamic_object_size can always deduce the size of every object passed to it so systemd can end up using either malloc_usable_size or __builtin_dynamic_object_size when pointers are passed around, which in turn can lead to actual segfaults like the one mentioned in systemd#23619. Apparently __builtin_object_size can return different results for pointers referring to the same memory as well but somehow it hasn't caused any issues yet. Looks like this whole malloc_usable_size/FORTIFY_SOURCE stuff should be revisited. Closes systemd#23619 and systemd#23150. Reopens systemd#22801
…_size." This reverts commit 0bd2925. It isn't guaranteed anywhere that __builtin_dynamic_object_size can always deduce the size of every object passed to it so systemd can end up using either malloc_usable_size or __builtin_dynamic_object_size when pointers are passed around, which in turn can lead to actual segfaults like the one mentioned in systemd/systemd#23619. Apparently __builtin_object_size can return different results for pointers referring to the same memory as well but somehow it hasn't caused any issues yet. Looks like this whole malloc_usable_size/FORTIFY_SOURCE stuff should be revisited. Closes systemd/systemd#23619 and systemd/systemd#23150. Reopens systemd/systemd#22801 (cherry picked from commit 2cfb790)
…_size." This reverts commit 0bd2925. It isn't guaranteed anywhere that __builtin_dynamic_object_size can always deduce the size of every object passed to it so systemd can end up using either malloc_usable_size or __builtin_dynamic_object_size when pointers are passed around, which in turn can lead to actual segfaults like the one mentioned in systemd/systemd#23619. Apparently __builtin_object_size can return different results for pointers referring to the same memory as well but somehow it hasn't caused any issues yet. Looks like this whole malloc_usable_size/FORTIFY_SOURCE stuff should be revisited. Closes systemd/systemd#23619 and systemd/systemd#23150. Reopens systemd/systemd#22801 (cherry picked from commit 2cfb790)
FYI, I started a conversation on malloc_usable_size here: https://sourceware.org/pipermail/libc-alpha/2022-November/143599.html which continued here: https://sourceware.org/pipermail/libc-alpha/2022-December/143667.html where we discuss systemd's use of
I think the safest way forward here for systemd is to wean itself away from |
When we asked the last time glibc people said everything is fine and we can rely on it. Really sucks if you then suddenly deprecate it without replacement. Why not just make it return the actually safe size? Then everything would just work. We are not using this just as an optimization, we are using it because it makes many things so kuch easier (for example a free() wrapper call that erases the memory). For that it would be entirely fine to just make the call return the exact size originally passed. But just breaking things without replacement after we explicitly asked about it earlier and where our use was okeyed is just mean. |
To say this explicitly: we really dont care about using the extra space malloc_usable_size() returns. All we care about is knowing how much at least to erase. Give us that and we are happy. |
There was a misunderstanding about how systemd was using
Unfortunately, the size is currently not stored anywhere and doing so would result in an allocation overhead for all applications. Also, while this may fix it for glibc, the key problem is that the interface does not guarantee that the return value from
I can assure you that's not the spirit with which we're doing this. I can't speak for previous endorsements because I didn't make them, but AFAICT, they don't seem to speak for the design pattern at all, they talk more about the allocator interface contract re. I've been trying to think of alternatives since this bug was filed (see siddhesh@e6760d4 or the patch I posted in my email to libc-alpha for example, which could help fix the undefined behaviour issue but not the issue of arbitrarily changing chunk sizes) and really the cleanest and most portable way I can think of is for systemd to start passing object sizes around instead of relying on |
I mean, come on, maintaining the size in systemd means basically putting a second layer of memory management on top of glibcs. If that's what we have to do, I think I am more tempted to simply not use glibc's allocator at all anymore. I mean, maintaining our own metadata for every allocation is just nuts. I mean, it appears to me that you have your FORTIFY_SOURCE=3 thing as a goal and that's the only thing that matters, and you really don#t care about anyone actually using this the stuff, i.e. our usecase is just not interesting to you. I mean, please understand: we don't really care about the size per se. What we care about most importantly is memory allocations which get automatically erase on free(). We can certainly give up on that idea, on the altar of FORTIFY_SOURCE=3, but it should be very clear that this is a serious degradation of security, and in theory on the altar of FORTIFY_SOURCE=3. Anyway, what about putting usability in the foreground, and all security instead of throwing it all out of the window for a much less interesting concept of security, i.e. FORTIFY_SOURCE=3? |
Also, can you explain to me, why it's such a terrible idea to just make malloc_usable_size() work as one would expect? I mean, apparently FORITFY_SOURCE=3 has a supposedly good idea of the expected size of memory allocations. So why in heaven can't malloc_usable_size() return a size that takes that information into account? Why do you think it's a good idea to know the right size, but under no circumstances allowing code to actually make use of that information safely? I mean, what's driving this? |
I doubt that, systemd seems to be the only application in all of the distribution (that I tested and evidently, Suse too) that seems to trip on this and surely everyone doesn't have their own allocator. Passing an array along with its size doesn't mean managing ones own allocations.
The systemd use case is that it is risky for reasons that I already explained a couple of times above. Whether it is interesting to me or not is not really the point because I'm not personally invested in seeing
Let me look at that more closely and see if there's anything I can help with.
I'm not sure how keeping
It's because they're two different sources of truth, one from allocator internals and another from whatever the compiler can glean from the code it has visibility to. Also, making
It's not a good idea, that's why it's being deprecated. I don't have the context on when the API was added, but given the programming practice warning, it never seems to have been a desired intent. |
Apart from erasing before free, we also use |
Is that for performance? I've added a lockless short-circuit to make that path faster in glibc malloc, so that shouldn't be necessary anymore.
That's a fair point.
We've talked about a new interface that returns the actually allocated size, there are standards proposals on those lines for C++ too. I suppose to support something like that we'll need to keep a consistent internal record of size somewhere anyway, that we can then pass out to callers. Let me think about this. |
So you are concerned about two sources of truth, but here you want to push everyone using your APIs to do this (i.e. malloc's own, and the user's assumption of the size), and that's risky as it could be. That's bad security engineering: making things complicated for users to use correctly, and easy to fuck up. Seriously, this just creates the desire to give up on this mess and start already with switching finally over to Rust. |
I don't follow. Why is this so problematic? Why can't make gcc make its knowledge available so that glibc can incorporate it into what it reports in malloc_usable_size()?
btw, what we actually want is some flag we can set on allocations that says "this is for sensitive" data, and that puts the allocation in locked memory and erase it on free automatically. I suggested this to glibc people somewhere (on some bugzilla, don't remember), but this was shot down, telling me to use openssl's secmem stuff instead, but dealing with two allocators in the same program is just a mess, and would probably result in security relevant bugs all the time. it's so weird that there's zero interest from glibc people to solve these — very important as I believe – things, but a lot of interest in breaking the approaches people find to work around them.
Why would two sources of truth actually have to be an issue? First of all, we already take the minimum of malloc_usable_size() and __builtin_object_size() for all our uses into account. We'd also use the dynamic version of the latter if it would actually work. Alas it appears to be fucked up, see that other PR. But more importantly: why doesn't glibc do this on its own? malloc_usable_size() could take this stuff into account too, if it wanted. BTW, I looked into replacing our use of malloc_usable_size() with a combination of malloc_usable_size() + realloc() so that we officialy tell glibc that we actually might write to the rest of the buffer. But when I toyed around it I realized glibc was lying and ended up copying the memory in this case sometimes. But if you'd fix that in glibc, i.e. that we get the guarantee that
Maybe try to turn this around: would it be good if more people would use malloc_usable_size() properly like we do? I think absolutely, yes, it would be. It improves security, and robustness, it removes secondary sources of truth, secondary memory management bookkeeping. So instead of just making clear to us that systemd is not reason enough to care to improve the situation, how about thinking what can you do to make programming with C safer and easier in general? having malloc_usable_size() that works would certainly achieve that. |
gcc gleans object sizes from the allocation points that it is able to see, i.e. the allocation has to be within the same translation unit and somehow associated with the pointer in the function that the compiler is currently building. For
I suppose something like Contexts for requests change over years and maybe what was deemed infeasible then could be worth revisiting today.
So the In the systemd context, it is unlikely for one function to see an object size as the result of
You can do that without calling realloc, see my PR #25688
I fixed that yesterday but it's not really necessary for systemd: https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=f4f2ca1509288f6f780af50659693a89949e7e46 |
So… why not just cut this short and not allow the allocator to ever change the usable size down? If "ever" is not possible, maybe make |
Shrinking is not so much a problem, given that the However, that's not the current behaviour; right now glibc malloc right now does not touch allocated blocks, so the usable size will remain the same until someone finds some performance opportunity there in future and then it all breaks again. After mulling a bit, I finally posted PR #25688 (looks like it has failed CI, I need to look at fixing that) because it fixes at least one problem, i.e. malloc_usable_size use with the current implementation of glibc |
systemd uses malloc_usable_size() everywhere to use memory blocks obtained through malloc, but that is abuse since the malloc_usable_size() interface isn't meant for this kind of use, it is for diagnostics only. This is also why systemd behaviour is flaky when built with _FORTIFY_SOURCE. One way to make this more standard (and hence safer) is to, at every malloc_usable_size() call, also 'reallocate' the block so that the compiler can see the larger size. This is done through a dummy reallocator whose only purpose is to tell the compiler about the larger usable size, it doesn't do any actual reallocation. Florian Weimer pointed out that this doesn't solve the problem of an allocator potentially growing usable size at will, which will break the implicit assumption in systemd use that the value returned remains constant as long as the object is valid. The safest way to fix that is for systemd to step away from using malloc_usable_size() like this. Resolves #22801.
…_size." This reverts commit 0bd292567a543d124cd303f7dd61169a209cae64. It isn't guaranteed anywhere that __builtin_dynamic_object_size can always deduce the size of every object passed to it so systemd can end up using either malloc_usable_size or __builtin_dynamic_object_size when pointers are passed around, which in turn can lead to actual segfaults like the one mentioned in systemd/systemd#23619. Apparently __builtin_object_size can return different results for pointers referring to the same memory as well but somehow it hasn't caused any issues yet. Looks like this whole malloc_usable_size/FORTIFY_SOURCE stuff should be revisited. Closes systemd/systemd#23619 and systemd/systemd#23150. Reopens systemd/systemd#22801
systemd uses malloc_usable_size() everywhere to use memory blocks obtained through malloc, but that is abuse since the malloc_usable_size() interface isn't meant for this kind of use, it is for diagnostics only. This is also why systemd behaviour is flaky when built with _FORTIFY_SOURCE. One way to make this more standard (and hence safer) is to, at every malloc_usable_size() call, also 'reallocate' the block so that the compiler can see the larger size. This is done through a dummy reallocator whose only purpose is to tell the compiler about the larger usable size, it doesn't do any actual reallocation. Florian Weimer pointed out that this doesn't solve the problem of an allocator potentially growing usable size at will, which will break the implicit assumption in systemd use that the value returned remains constant as long as the object is valid. The safest way to fix that is for systemd to step away from using malloc_usable_size() like this. Resolves systemd#22801. (cherry picked from commit 7929e18) (cherry picked from commit 34b9edd) (cherry picked from commit 70653eb)
systemd uses malloc_usable_size() everywhere to use memory blocks obtained through malloc, but that is abuse since the malloc_usable_size() interface isn't meant for this kind of use, it is for diagnostics only. This is also why systemd behaviour is flaky when built with _FORTIFY_SOURCE. One way to make this more standard (and hence safer) is to, at every malloc_usable_size() call, also 'reallocate' the block so that the compiler can see the larger size. This is done through a dummy reallocator whose only purpose is to tell the compiler about the larger usable size, it doesn't do any actual reallocation. Florian Weimer pointed out that this doesn't solve the problem of an allocator potentially growing usable size at will, which will break the implicit assumption in systemd use that the value returned remains constant as long as the object is valid. The safest way to fix that is for systemd to step away from using malloc_usable_size() like this. Resolves systemd#22801. (cherry picked from commit 7929e18)
The idea of nm_free_secret() is to clear the secrets from memory. That surely is some layer of extra snake oil, because we tend to pass secrets via D-Bus, where the memory gets passed down to (D-Bus) libraries which have no idea to keep it private. Still... But turns out, malloc_usable_size() might not actually be usable for this. Read the discussion at [1]. Stop using malloc_usable_size(), which seems unfortunate. There is probably no secret relevant data after the NUL byte anyway, because we tend to create such strings once, and don't rewrite/truncate them afterwards (which would leave secrets behind as garbage). Note that systemd's erase_and_free() still uses malloc_usable_size() ([2]) but the macro foo to get that right is terrifying ([3]). [1] systemd/systemd#22801 (comment) [2] https://github.com/systemd/systemd/blob/11c0f0659ecd82572c2dc83f3b34493a36dcd954/src/basic/memory-util.h#L101 [3] systemd/systemd@7929e18 Fixes: d63cd26 ('shared: improve nm_free_secret() to clear entire memory buffer')
The idea of nm_free_secret() is to clear the secrets from memory. That surely is some layer of extra snake oil, because we tend to pass secrets via D-Bus, where the memory gets passed down to (D-Bus) libraries which have no idea to keep it private. Still... But turns out, malloc_usable_size() might not actually be usable for this. Read the discussion at [1]. Stop using malloc_usable_size(), which seems unfortunate. There is probably no secret relevant data after the NUL byte anyway, because we tend to create such strings once, and don't rewrite/truncate them afterwards (which would leave secrets behind as garbage). Note that systemd's erase_and_free() still uses malloc_usable_size() ([2]) but the macro foo to get that right is terrifying ([3]). [1] systemd/systemd#22801 (comment) [2] https://github.com/systemd/systemd/blob/11c0f0659ecd82572c2dc83f3b34493a36dcd954/src/basic/memory-util.h#L101 [3] systemd/systemd@7929e18 Fixes: d63cd26 ('shared: improve nm_free_secret() to clear entire memory buffer') (cherry picked from commit 8b66865)
The idea of nm_free_secret() is to clear the secrets from memory. That surely is some layer of extra snake oil, because we tend to pass secrets via D-Bus, where the memory gets passed down to (D-Bus) libraries which have no idea to keep it private. Still... But turns out, malloc_usable_size() might not actually be usable for this. Read the discussion at [1]. Stop using malloc_usable_size(), which seems unfortunate. There is probably no secret relevant data after the NUL byte anyway, because we tend to create such strings once, and don't rewrite/truncate them afterwards (which would leave secrets behind as garbage). Note that systemd's erase_and_free() still uses malloc_usable_size() ([2]) but the macro foo to get that right is terrifying ([3]). [1] systemd/systemd#22801 (comment) [2] https://github.com/systemd/systemd/blob/11c0f0659ecd82572c2dc83f3b34493a36dcd954/src/basic/memory-util.h#L101 [3] systemd/systemd@7929e18 Fixes: d63cd26 ('shared: improve nm_free_secret() to clear entire memory buffer') (cherry picked from commit 8b66865) (cherry picked from commit 6e7fb78)
systemd uses malloc_usable_size() everywhere to use memory blocks obtained through malloc, but that is abuse since the malloc_usable_size() interface isn't meant for this kind of use, it is for diagnostics only. This is also why systemd behaviour is flaky when built with _FORTIFY_SOURCE. One way to make this more standard (and hence safer) is to, at every malloc_usable_size() call, also 'reallocate' the block so that the compiler can see the larger size. This is done through a dummy reallocator whose only purpose is to tell the compiler about the larger usable size, it doesn't do any actual reallocation. Florian Weimer pointed out that this doesn't solve the problem of an allocator potentially growing usable size at will, which will break the implicit assumption in systemd use that the value returned remains constant as long as the object is valid. The safest way to fix that is for systemd to step away from using malloc_usable_size() like this. Resolves systemd#22801.
It might very well return a value larger than the actual usable size, so writing to the excess bytes is Undefined Behavior. There's absolutely no promise about the value, except that it is no less than the size that was once passed to malloc(3). Link: <systemd/systemd#22801 (comment)> Link: <https://inbox.sourceware.org/libc-alpha/20221124213258.305192-1-siddhesh@gotplt.org/T/> Reported-by: Mingye Wang <arthur200126@gmail.com> Reported-by: Siddhesh Poyarekar <siddhesh@gotplt.org> Cc: DJ Delorie <dj@redhat.com> Cc: Sam James <sam@gentoo.org> Cc: Florian Weimer <fweimer@redhat.com> Cc: Andreas Schwab <schwab@linux-m68k.org> Cc: Zack Weinberg <zack@owlfolio.org> Cc: Wilco Dijkstra <Wilco.Dijkstra@arm.com> Signed-off-by: Alejandro Colomar <alx@kernel.org>
systemd uses malloc_usable_size() everywhere to use memory blocks obtained through malloc, but that is abuse since the malloc_usable_size() interface isn't meant for this kind of use, it is for diagnostics only. This is also why systemd behaviour is flaky when built with _FORTIFY_SOURCE. One way to make this more standard (and hence safer) is to, at every malloc_usable_size() call, also 'reallocate' the block so that the compiler can see the larger size. This is done through a dummy reallocator whose only purpose is to tell the compiler about the larger usable size, it doesn't do any actual reallocation. Florian Weimer pointed out that this doesn't solve the problem of an allocator potentially growing usable size at will, which will break the implicit assumption in systemd use that the value returned remains constant as long as the object is valid. The safest way to fix that is for systemd to step away from using malloc_usable_size() like this. Resolves systemd#22801. (cherry picked from commit 7929e18) (cherry picked from commit 34b9edd)
Using the new
-D_FORTIFY_SOURCE
level, I see the following buffer overflow:It happens here:
systemd/src/basic/fileio.c
Lines 469 to 482 in e7949be
where
MALLOC_SIZEOF_SAFE
is defined as:systemd/src/basic/alloc-util.h
Lines 173 to 183 in 02036ca
Even though, the comment counts with
-D_FORTIFY_SOURCE
it behaves wrongly asSIZE_MAX
is returned for an unknown object. Thus we end up withsize == malloc_usable_size
and the assert happens.There's a reduced test-case:
I think one possible fix would be usage of
__builtin_dynamic_object_size
, or notusing
malloc_usable_size
at all.CCing GCC developer of the new fortification level @siddhesh.
The text was updated successfully, but these errors were encountered: