After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 746048 - parsing an unclosed comment can result in `Conditional jump or move depends on uninitialised value(s)` and unsafe memory access
parsing an unclosed comment can result in `Conditional jump or move depends o...
Status: RESOLVED FIXED
Product: libxml2
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: Daniel Veillard
libxml QA maintainers
Depends on:
Blocks:
 
 
Reported: 2015-03-11 20:04 UTC by Mike Dalessio
Modified: 2017-08-14 05:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
suggested patch from Francois Chagnon (2.77 KB, patch)
2015-03-11 20:04 UTC, Mike Dalessio
none Details | Review

Description Mike Dalessio 2015-03-11 20:04:49 UTC
Created attachment 299127 [details] [review]
suggested patch from Francois Chagnon

The following code, when compiled and linked against libxml2 master (I'm on 02b252d), will result in valgrind errors.

```
#include "string.h"

#include <libxml/HTMLparser.h>
#include <libxml/HTMLtree.h>

int main(int argc, char** argv)
{
  // Nokogiri::HTML::fragment("<!-- ")

  htmlDocPtr doc ;
  int options = HTML_PARSE_RECOVER | HTML_PARSE_NOERROR | HTML_PARSE_NOWARNING | HTML_PARSE_NONET ;

  char* HTMLFRAG_GOOD = "<html><body><!-- x" ;
  char* HTMLFRAG_BAD  = "<html><body><!--" ;

  char* HTMLFRAG = HTMLFRAG_BAD ;

  xmlInitParser();

  doc = htmlReadMemory(HTMLFRAG, strlen(HTMLFRAG),
                       NULL,
                       "UTF-8",
                       options);

  xmlFreeDoc(doc);
}
```

There are 13 different error contexts reported by valgrind on my system, but all are variations on "Conditional jump or move depends on uninitialised value(s)", for example:

```
==32338== Conditional jump or move depends on uninitialised value(s)
==32338==    at 0x43C377: htmlCurrentChar (HTMLparser.c:440)
==32338==    by 0x43CF4A: htmlParseComment (HTMLparser.c:3250)
==32338==    by 0x44012E: htmlParseContentInternal (HTMLparser.c:4559)
==32338==    by 0x440C0E: htmlParseDocument (HTMLparser.c:4735)
==32338==    by 0x443741: htmlDoRead (HTMLparser.c:6707)
==32338==    by 0x402B39: main (in /home/miked-public/code/oss/nokogiri/issue-xxx-libxml/repro)
```

This issue was originally reported by Florian Weingarten.

A patch was suggested by Francois Chagnon, which prevents the errors seen by valgrind on my system and thus might be of use to you. I have attached it as `read-past-buffer.diff`.

Thanks for your time and consideration,
-mike
Comment 1 Mike Dalessio 2015-03-11 20:16:50 UTC
Possibly also worth noting, for prioritization purposes, is that the original reporter was seeing real unsafe memory access (not simply a use of uninitialized pointers) in a production system.

I suspect there may be a DOS vector here, but couldn't produce on on my local system in a reasonable amount of time.
Comment 2 Daniel Veillard 2015-03-14 14:39:21 UTC
Hum, I need to look at this thanks for raising this, and the patch :-) !

Daniel
Comment 3 Jun Kokatsu 2015-05-11 18:21:18 UTC
Dear All,

I'm founder of this bug.

https://hackerone.com/reports/57125

In Shopify, I was able to see previous http request which includes Email address, session_hash, and even last 4 digit of credit card number. I reported this bug on 8th of March and reported to Shopify but I didn't know that it was problem of this. Just FYI.

Thanks,

Jun
Comment 4 Daniel Veillard 2015-10-30 11:15:56 UTC
Dohh it seems I dropped the ball ... Looking at it !

Daniel
Comment 5 Daniel Veillard 2015-10-30 13:24:45 UTC
I think the fundamental problem is that a lot of the code assumes 0 terminated buffers, the problem is that the HTML parser there forgot to check for the
termination when reading the 2 first characters in the comment.
As a result I ended up with a rather different patch

https://git.gnome.org/browse/libxml2/commit/?id=e724879d964d774df9b7969fc846605aa1bac54c

  thanks for the report and the initial patch, sorry it took so long !

Daniel
Comment 6 Mike Dalessio 2015-10-30 20:59:05 UTC
Great, thank you!
Comment 7 Reed Loden 2015-12-31 22:03:31 UTC
This has been assigned CVE-2015-8710 by MITRE.