Better error reporting

[fereidani]

Hi ,

I find some possible security vulnerabilities in this package .

some uncommon inputs for RE2 like arrays and objects cause crash in main node process .

Here is 2 examples :

var regex=new RE2([])

FATAL ERROR: v8::Object::GetAlignedPointerFromInternalField() Internal field out of bounds Aborted (core dumped)

var regex=new RE2({})

Segmentation fault (core dumped)

My test platform is ubuntu , node version v6.0.0

Best Regards

[uhop]

How is it a security vulnerability? How an end user or a malicious hacker can possibly provide "uncommon" parameters for a constructor? It looks to me more like a programmer's error of GIGO variety.

Now it may make sense to validate input parameters so instead of a core dump, it will barf an exception. Arguably it is a better developer's experience. I'll look into that.

[fereidani]

Javascript/V8 is memory safe language , so any package should avoid process level exceptions and guaranty memory safety.

possible scenario for attack is a web application that receive regex input from user , now if a hacker find this functionality can destroy process repeatedly and perform serious denial of service .

i didn't read code and don't know what cause the crash , usually memory caused exceptions lead to remote command execution , as javascript is easily vulnerable to heap spray it's very possible to an attacker to turn this beyond just denial of service .

normal behavior of javascript default regex is :

new RegExp([])
/(?:)/

new RegExp({})
/[object Object]/

i suggest this behavior in library , to provide better compatibility too .

[uhop]

What regex precisely causes a segmentation fault? Please share.

I don't think anyone can cause a remote command execution after a segmentation fault.

As to more graceful errors — I agree. Still I don't see an example from you how it is a security problem. Do you allow end users to enter JavaScript directly anywhere? Just code right, and don't use non-sensical initializers.

I checked the latest standard on RegExp, and can't find any wording on how and why it should be initialized with arrays, nor objects. Please explain.

[fereidani]

as i shared in first post , these 2 regex cause crash : var regex=new RE2([]) var regex=new RE2({})

and second one cause segmentation fault ,

no , code execution can happened before segmentation fault by getting control of CPU Instruction Pointer (you can look at security articles for more information) . as i said i didn't read code that cause this problem so i don't know type of memory corruption ( buffer overflow , heap overflow , dangling pointer , null pointer dereference , ..... ) so i can't tell what is exploit method could cause remote code execution , and sorry i don't really have time to find .

it's not standard behavior of v8 to crash a process with any input (whether sensical or unsensical ) and any extending package must preserve it .

i know it's not sensical to pass a array or object to regex constructor , but package should handle any illegal and irrational input without causing crash .

how things can go wrong ?

1.some one has a web application and handle financial transactions 2.he get a form as json object 3.field regex is your regex rule as string 4.regex key passed to RE2

1. attacker find it
2. pass a object or array as regex key
3. cause crash
4. financial transactions lost
5. process and some one destroyed both :D

[uhop]

I venture to say that examples that cause crash are not regular expressions, just a random garbage that makes no sense. Having a crash right there will actually help to fix the bug, rather than to gloss it over like node.js does by applying toString() to a non-sensical input. BTW, this is why new RegExp({}) is the same as new RegExp("[objectObject]").

Regarding the JSON example: no, regular expression object cannot be a part of JSON (see the standard). Always validate your input, do not rely on others to do it.

Thank you for alerting me about a possible abuse, but it is clearly not a security bug, but rather an enhancement proposal. While it is clearly documented how RE2 can be initialized properly, many people are writers, not readers. Until the check is implemented, I suggest to use a regular RegExp, which comes with the standard library, and has no problems mentioned in this ticket.

[davisjam]

I agree that this is a bug, but calling it a security vulnerability seems far-fetched.

To be vulnerable, the application must invoke RE2 with an empty object or array. Even were the application foolish enough to pass unfiltered user input to this constructor, it would presumably be doing so with a string from the user ('{}', '[]') rather than a raw object or array.

It's entirely possible, however, that the application might have an incorrect ("insecure") call to RE2 behind a conditional that an attacker could tweak. If the application test suite doesn't check this path, an attacker could bring down the application by routing the control flow into the block with the incorrect code.

@uhop GIGO is one thing, but segfaulting is a pretty extreme "garbage out". I think throwing an exception would be a suitable solution. Would you like me to cook up a PR?

[uhop]

Please do.

[uhop]

We may need to identify what other errors (besides #9) we want to detect, and report. Specifically we should take care of anything that causes coredumps.