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

[CVE-2024-28757] Prevent billion laughs attacks in isolated external parser (part of #839) #842

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

hartwork
Copy link
Member

@hartwork hartwork commented Mar 5, 2024

Part of #839

CC (in alphabetic order) @catenacyber @RMJ10 @Snild-Sony

@hartwork hartwork added this to the 2.6.2 milestone Mar 5, 2024
@hartwork hartwork mentioned this pull request Mar 6, 2024
28 tasks
@Snild-Sony
Copy link
Contributor

I would like a lot more info in the fix's commit message: how does the problem happen, and why does the commit fix it?

Also, is it even valid to run an external parser without having parsed a single byte of the parent document? Is there a valid usecase for that, or should it just be disallowed?

@hartwork
Copy link
Member Author

hartwork commented Mar 6, 2024

I would like a lot more info in the fix's commit message: how does the problem happen, and why does the commit fix it?

@Snild-Sony that's a valid point. I'm a bit shy with prose that goes into public Git history forever and find out a week later that something was off. I'll look into it 👍

Also, is it even valid to run an external parser without having parsed a single byte of the parent document? Is there a valid usecase for that, or should it just be disallowed?

Those are good questions. The API allows it and the current fuzzers do exactly that, which put a spotlight to it. Some XML IDE could do that maybe to parse a known-DTD file even without a user, not sure.

@Snild-Sony
Copy link
Contributor

I'm a bit shy with prose that goes into public Git history forever

As someone who does a bunch of debugging in unfamiliar code written by others, I'll say that good commit messages are extremely helpful in understanding why some piece of code is the way it is.

Think of it as "this is what I believed at the time of making this change". If it turns out to be wrong, that's a good thing, because it's a strong signal that the change itself might also need adjustment.

@hartwork
Copy link
Member Author

hartwork commented Mar 6, 2024

I'm a bit shy with prose that goes into public Git history forever

As someone who does a bunch of debugging in unfamiliar code written by others, I'll say that good commit messages are extremely helpful in understanding why some piece of code is the way it is.

Think of it as "this is what I believed at the time of making this change". If it turns out to be wrong, that's a good thing, because it's a strong signal that the change itself might also need adjustment.

@Snild-Sony I agree but the picture misses public perception and reputation. If I'm wrong, I'd rather be wrong in private. Could be cultural.

@RMJ10
Copy link
Contributor

RMJ10 commented Mar 6, 2024

Would it be acceptable to reference (and preferably summarise) #839? That would at least give future debuggers (future you!) somewhere to start.

@hartwork hartwork force-pushed the issue-839-billion-laughs-isolated-external-parser branch from 20bb257 to 58431b0 Compare March 6, 2024 22:37
@hartwork
Copy link
Member Author

hartwork commented Mar 6, 2024

@Snild-Sony @RMJ10 you're right, I have now extended the commit message as following:

    lib/xmlparse.c: Detect billion laughs attack with isolated external parser
    
    When parsing DTD content with code like ..
    
      XML_Parser parser = XML_ParserCreate(NULL);
      XML_Parser ext_parser = XML_ExternalEntityParserCreate(parser, NULL, NULL);
      enum XML_Status status = XML_Parse(ext_parser, doc, (int)strlen(doc), XML_TRUE);
    
    .. there are 0 bytes accounted as direct input and all input from `doc` accounted
    as indirect input.  Now function accountingGetCurrentAmplification cannot calculate
    the current amplification ratio as "(direct + indirect) / direct", and it did refuse
    to divide by 0 as one would expect, but it returned 1.0 for this case to indicate
    no amplification over direct input.  As a result, billion laughs attacks from
    DTD-only input were not detected with this isolated way of using an external parser.
    
    The new approach is to assume direct input of length not 0 but 22 -- derived from
    ghost input "<!ENTITY a SYSTEM 'b'>", the shortest possible way to include an external
    DTD --, and do the usual "(direct + indirect) / direct" math with "direct := 22".
    
    GitHub issue #839 has more details on this issue and its origin in ClusterFuzz
    finding 66812.

…arser

When parsing DTD content with code like ..

  XML_Parser parser = XML_ParserCreate(NULL);
  XML_Parser ext_parser = XML_ExternalEntityParserCreate(parser, NULL, NULL);
  enum XML_Status status = XML_Parse(ext_parser, doc, (int)strlen(doc), XML_TRUE);

.. there are 0 bytes accounted as direct input and all input from `doc` accounted
as indirect input.  Now function accountingGetCurrentAmplification cannot calculate
the current amplification ratio as "(direct + indirect) / direct", and it did refuse
to divide by 0 as one would expect, but it returned 1.0 for this case to indicate
no amplification over direct input.  As a result, billion laughs attacks from
DTD-only input were not detected with this isolated way of using an external parser.

The new approach is to assume direct input of length not 0 but 22 -- derived from
ghost input "<!ENTITY a SYSTEM 'b'>", the shortest possible way to include an external
DTD --, and do the usual "(direct + indirect) / direct" math with "direct := 22".

GitHub issue #839 has more details on this issue and its origin in ClusterFuzz
finding 66812.
@hartwork hartwork force-pushed the issue-839-billion-laughs-isolated-external-parser branch from 58431b0 to 072eca0 Compare March 6, 2024 22:41
Copy link
Contributor

@Snild-Sony Snild-Sony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great commit message, making both the problem and the solution very clear.

@hartwork hartwork merged commit 5026213 into master Mar 7, 2024
68 checks passed
@hartwork hartwork changed the title Prevent billion laughs attacks in isolated external parser (part of #839) [CVE-REQUESTED] Prevent billion laughs attacks in isolated external parser (part of #839) Mar 8, 2024
@hartwork hartwork deleted the issue-839-billion-laughs-isolated-external-parser branch March 8, 2024 11:56
@carnil
Copy link

carnil commented Mar 10, 2024

CVE-2024-28757 seems related to this issue.

@hartwork
Copy link
Member Author

@carnil I confirm, found it in my mailbox from ~7 hours ago.

@hartwork hartwork changed the title [CVE-REQUESTED] Prevent billion laughs attacks in isolated external parser (part of #839) [CVE-2024-28757] Prevent billion laughs attacks in isolated external parser (part of #839) Mar 10, 2024
@hartwork
Copy link
Member Author

@carnil PS: Releasing 2.6.2 with this fix on Wednesday 2024-03-13 UTC+1 evening is the current plan — could be sooner or later (..) — but that's the plan.

@hartwork
Copy link
Member Author

@carnil 2.6.2 is out by now

sgunin pushed a commit to sgunin/oe-openembedded-core-contrib that referenced this pull request Mar 17, 2024
Picked patch from libexpat/libexpat#842
which is referenced in the NVD CVE report.

Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Steve Sakoman <steve@sakoman.com>
halstead pushed a commit to yoctoproject/poky that referenced this pull request Mar 25, 2024
Picked patch from libexpat/libexpat#842
which is referenced in the NVD CVE report.

(From OE-Core rev: c02175e97348836429cecbfad15d89be040bbd92)

Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Steve Sakoman <steve@sakoman.com>
jpuhlman pushed a commit to MontaVista-OpenSourceTechnology/poky that referenced this pull request Apr 2, 2024
Source: poky
MR: 132404, 131331
Type: Security Fix
Disposition: Merged from poky
ChangeID: fe9d4cb
Description:

Picked patch from libexpat/libexpat#842
which is referenced in the NVD CVE report.

(From OE-Core rev: c02175e97348836429cecbfad15d89be040bbd92)

Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Steve Sakoman <steve@sakoman.com>
Signed-off-by: Jeremy A. Puhlman <jpuhlman@mvista.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants