Comment by codedokode
2 months ago
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.
> It is a valid bug.
But is it a high-severity security bug?
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.
Considering that it's Undefined Behavior, quite possibly.
EDIT: That said, I'm on the maintainer's side here.
> 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.
4 replies →
> 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.
So could the reporter of the bug. Alternatively, he could add an `if(is null){crash}` after the malloc. The fix is easy for anyone that has some knowledge of the code base. The reporter has demonstrated this knowledge in finding the issue.
If a useful PR/patch diff was provided with the reporter, I would have expected it to be merged right away.
However, instead of doing the obvious thing to actually solve the issue, the reporter hits the maintainer with this bureaucratic monster:
> We'd like to inform you that we are preparing publications on the discovered vulnerability.
> Our Researchers plan to release the technical research, which will include the description and details of the discovered vulnerability.
> The research will be released after 90 days from the date you were informed of the vulnerability (approx. August 5th, 2025).
> Please answer the following questions:
>> * When and in what version will you fix the vulnerability described in the Report? (date, version)
> * If it is not possible to release a patch in the next 90 days, then please indicate the expected release date of the patch (month).
> * Please, provide the CVE-ID for the vulnerability that we submitted to you.
>> In case you have any further questions, please, contact us.
https://gitlab.gnome.org/GNOME/libxml2/-/issues/905#note_243...
The main issue here is really one of tone. The maintainer has been investing his free time to altruistically move the state of software forward and the reporter is too lazy to even type up a tone-adjusted individual message. Would it have been so hard for the reporter to write the following?
> Thank you for your nice library. It is very useful to us! However, we found a minor error that unfortunately might be severely exploitable. Attached is a patch that "fixes" it in an ad-hoc way. If you want to solve the issue in a different way, could we apply the patch first, and then you refactor the solution when you find time? Thanks! Could you give us some insights on when after merging to main/master, the patch will end up in a release? This is important for us to decide whether we need to work with a bleeding edge master version. Thank you again for your time!
Ultimately, it is a very similar message content. However, it feels completely different.
Suppose you are a maintainer without that much motivation left, and you get hit with such a message. You will feel like the reporter is an asshole. (I'm not saying he is one.) Do you really care, if he gets powned via this bug? It takes some character strength on the side of the maintainer to not just leave the issue open out of spite.
> the reporter is too lazy to even type up a tone-adjusted individual message. Would it have been so hard for the reporter to write the following?
The reporter doesn't care about libxml2 being more secure, they only care about having a CVE-ID to brag about discovering a vulnerability and publishing it on their blog. If the reporter used the second message you wrote, they wouldn't get what they want.
If someone had reported that on a project I maintain, I'd have told them to get outta here, in somewhat less polite language. They're quite clearly promoting their own company / services and don't care in the slightest about libxml2.
I mean, no security researchers do. It's very much like capitalists. They aren't trying to do something to improve society, but by persuing their own private incentives, they end up with behaviour that benefits the commons. Sometimes we need regulations around that in the marketplace, and that's what the FTC is. So we need an OSS-social-contract version of that.
It's kind of like the enshitification of bug reports. The best way to deal with it is probably denying them CVE numbers to disincentivise the look of low hanging fruit that reasonably could be done by a linter.
Reminds me of students juicing their PRs be making changes to typos in documentation and comments just to put it on their resumes.
2 replies →
If I received an email like that I'd reply with an invoice.
Many systems have (whether you like the idea or not) effectively infallible allocators. If malloc won't ever return null, there's not much point in checking.
A while back i remember looking at the kernel source code, when overcommit is enabled, malloc would not fail if it couldnt allocate memory, it would ONLY fail if you attempted to allocate memory larger than the available memory space.
I not think you can deal with the failure condition the way you think on Linux (and I imagine other operating systems too).
It's very easy to make malloc return NULL:
The bug was about the case when malloc returns null, but the library doesn't check for it.
Correct, but the point is that it is difficult to get malloc to return null on Linux. Why litter your code with checks for de facto impossible scenarios?
2 replies →
in the event that malloc returns NULL and it isn't checked, isn't the program going to crash anyways? I usually just use a macro like "must_malloc" that does this anyways. But the out come is the same I would think. It's mostly a difference of where it happens.