←back to thread

Libxml2's "no security embargoes" policy

(lwn.net)
298 points jwilk | 8 comments | 25 Jun 25 19:36 UTC | HN request time: 1.464s | source | bottom
Show context
arp242 ◴[25 Jun 25 22:06 UTC] No.44382233[source]▶
A lot of these "security bugs" are not really "security bugs" in the first place. Denial of service is not resulting in people's bank accounts being emptied or nude selfies being spread all over the internet.

Things like "panics on certain content" like [1] or [2] are "security bugs" now. By that standard anything that fixes a potential panic is a "security bug". I've probably fixed hundreds if not thousands of "security bugs" in my career by that standard.

Barely qualifies as a "security bug" yet it's rated as "6.2 Moderate" and "7.5 HIGH". To say nothing of gazillion "high severity" "regular expression DoS" nonsense and whatnot.

And the worst part is all of this makes it so much harder to find actual high-severity issues. It's not harmless spam.

[1]: https://github.com/gomarkdown/markdown/security/advisories/G...

[2]: https://rustsec.org/advisories/RUSTSEC-2024-0373.html

replies(14): >>44382268 #>>44382299 #>>44382855 #>>44384066 #>>44384368 #>>44384421 #>>44384513 #>>44384791 #>>44385347 #>>44385556 #>>44389612 #>>44390124 #>>44390292 #>>44396733 #
codedokode ◴[26 Jun 25 05:46 UTC] No.44384513[source]▶
Dereferencing a null pointer is an error. It is a valid bug.

The maintainer claims this is caused by allocator failure (malloc returning null), but it is still a valid bug. If you don't want to deal with malloc failures, just crash when malloc() returns null, instead of not checking malloc() result at all.

The maintainer could just write a wrapper around malloc that crashes on failure and replace all calls with the wrapper. It seems like an easy fix. Because almost no software can run where there is no heap memory so it makes no sense for the program to continue.

Another solution is to propagate every error back to the caller, but it is difficult and there is high probability that the caller won't bother checking the result because of laziness.

A quote from a bug report [1]:

> If xmlSchemaNewValue returns NULL (e.g., due to a failure of malloc), xmlSchemaDupVal checks for this and returns NULL.

[1] https://gitlab.gnome.org/GNOME/libxml2/-/issues/905

replies(5): >>44384716 #>>44385168 #>>44385200 #>>44386885 #>>44387099 #
1. saurik ◴[26 Jun 25 07:57 UTC] No.44385200[source]▶
> It is a valid bug.

But is it a high-severity security bug?

replies(2): >>44385763 #>>44401512 #
2. Quekid5 ◴[26 Jun 25 09:48 UTC] No.44385763[source]▶
Considering that it's Undefined Behavior, quite possibly.

EDIT: That said, I'm on the maintainer's side here.

replies(1): >>44386062 #
3. gpderetta ◴[26 Jun 25 10:43 UTC] No.44386062[source]▶
> Considering that it's Undefined Behavior, quite possibly.

Is it thought? Certainly it is according the C and C++ standards, but POSIX adds:

> References to unmapped addresses shall result in a SIGSEGV signal

While time-traveling-UB is a theoretical possibility, practically POSIX compliant compilers won't reorder around potentially trapping operations (they will do the reverse, they might remove a null check if made redundant by a prior potentially trapping dereference) .

A real concern is if a null pointer is dereferenced with a large attacker-controlled offset that can avoid the trap, but that's more of an issue of failing to bound check.

replies(1): >>44386336 #
4. ynik ◴[26 Jun 25 11:30 UTC] No.44386336{3}[source]▶
Under your interpretation, neither gcc nor clang are POSIX compliant. Because in practice all these optimizing compilers will reorder memory accesses without bothering to prove that the pointers involved are valid -- the compiler just assumes that the pointers are valid, which is justified because otherwise the program would have undefined behavior.
replies(2): >>44386394 #>>44386634 #
5. gpderetta ◴[26 Jun 25 11:38 UTC] No.44386394{4}[source]▶
Actually you are right, what I said about reordering is nonsense. The compiler will definitely reorder non-aliasing accesses. There are much weaker properties that are preserved.
6. somat ◴[26 Jun 25 12:11 UTC] No.44386634{4}[source]▶
I am not so sure. Assuming the program does not do anything undefined is sort of the worst possible take on leaving the behavior undefined in the first place. I mean the behavior was left undefined so that "something" could be done. the language standard just does not know what that "something" is. Hell the compiler could do nothing and that would make more sense.

But to make optimizations pretending it is an invariant, it can't happen, when the specification clearly says it could happen. That's wild, and I would argue out of specification.

replies(1): >>44408996 #
7. charleslmunger ◴[28 Jun 25 00:37 UTC] No.44401512[source]▶
If there was another bug that allowed input data to request a large allocation, this would be?

Normally I'd want a secure parser to allocate memory with a bound, and one way to implement that is to give each parse call an allocator that fails once it reaches a limit. From a very basic scan it seems like libxml2 supports swapping its allocator function, so it could do this; although that's probably not the intended purpose since those APIs are global rather than per parser instance.

I bet what happened here is the reporter ran a fuzzer, found a report, and didn't realize that the fuzzer configuration was injecting faults into malloc so they thought it was more severe than it actually was. Missing a bounds check often results in some massive number being passed to malloc, which then returns null, so this is a common pattern to find. It's not a high severity security bug but it's reasonable to think it could be one if you didn't know about the fault injecting allocator.

8. Quekid5 ◴[28 Jun 25 23:20 UTC] No.44408996{5}[source]▶
Undefined Behavior has a very specific meaning in the C and (respectively) C++ standards. And it is this.

I think you may be misunderstand how optimization works. It's not "AHA! Undefined Behavior, I'll poke at that!".

It's more that compilers are allowed to assume that some things just "don't happen"... and can optimize based on that. And it does make sense... if you were told to optimize a thing given certain rules, how would you do it?