Skip to content

Commit

Permalink
Fix crash because the GET handler sets the bosh_request_parser to nul…
Browse files Browse the repository at this point in the history 
…l. Also fix the case where the user might send malicious XML to the BOSH proxy
  • Loading branch information
@dhruvbird
dhruvbird committed Aug 28, 2012
1 parent dab8538 commit 9921051
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 11 deletions.
2 changes: 1 addition & 1 deletion src/bosh-request-parser.js
View file
Expand Up @@ -44,7 +44,7 @@ function BoshRequestParser() {
dutil.copy(BoshRequestParser.prototype, {
_handle_start_element: function(name, attrs) {
if (!this._started) {
if (name === "body") {
if (name.substr(0, 5) === "DUMMY") {
this._started = true;
} else {
this.end();
Expand Down
45 changes: 35 additions & 10 deletions src/http-server.js
View file
Expand Up @@ -44,15 +44,38 @@ function HTTPServer(port, host, stat_func, bosh_request_handler, http_error_hand

function parse_request(buffers) {
var valid_request = true;
for (var i = 0, len = buffers.length; i < len; i++) {
if (!valid_request) return null;

// We wrap every request in a <DUMMY_randomint> request
// </DUMMY_randomint> tag. This prevents the user from hacking
// the parser's stream by sending in a request like: <body>
// <blah/> </body> <body>
//
// If the user sent a reuqest like <body> <blah/> <DUMMY> or
// any such thing, then the parser will be able to detect it.
//
// We use a random (upto 7 digit) integer so that the attaker
// can not "guess" the name of the tag we are using to delimit
// the request.
//
var i;
var randInt = String(Math.floor(Math.random() * 1000000))
bosh_request_parser.parse('<DUMMY_' + randInt + '>');

for (i = 0; i < buffers.length; i++) {
// log.trace("Request fragment: %s", buffers[i]);
valid_request = bosh_request_parser.parse(buffers[i]);
if (!valid_request) return null;
}
valid_request = bosh_request_parser.parse('</DUMMY_' + randInt + '>');

if (valid_request && bosh_request_parser.parsedBody) {
return bosh_request_parser.parsedBody;
if (valid_request && bosh_request_parser.parsedBody
&& bosh_request_parser.parsedBody.getChild('body')) {
var bodyTag = bosh_request_parser.parsedBody.getChild('body');
bodyTag.parent = null;
return bodyTag;
} else {
bosh_request_parser = new BoshRequestParser();
return null;
}
}

Expand All @@ -63,15 +86,16 @@ function HTTPServer(port, host, stat_func, bosh_request_handler, http_error_hand
function handle_get_bosh_request(req, res, u) {
var ppos = u.pathname.search(bosh_options.path);
if (req.method === 'GET' && ppos !== -1 && u.query.hasOwnProperty('data')) {
if (!bosh_request_parser.parse(u.query.data)) {
req.destroy();
res = new helper.JSONPResponseProxy(req, res);
res.request_headers = req.headers;

if (parse_request([ u.query.data ]) === null) {
// FIXME: If we got an invalid JSON, we should respond
// with an error condition.
res.end("XML Parsing Error!");
} else {
res = new helper.JSONPResponseProxy(req, res);
res.request_headers = req.headers;
bosh_request_handler(res, bosh_request_parser.parsedBody);
}
bosh_request_parser.end();
bosh_request_parser = null;
return false;
}
}
Expand Down Expand Up @@ -101,6 +125,7 @@ function HTTPServer(port, host, stat_func, bosh_request_handler, http_error_hand
req_parts.forEach(function (p) {
log.warn("XML parsing Error: %s", p);
});
// FIXME: Send back valid XML.
res.end("XML parsing Error");
}
}
Expand Down

5 comments on commit 9921051

@satyamshekhar
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is almost perfect and covers all cases. We can get rid of the random number as well. What we need to add is -

  • In every call to parse set parsedBody to null, before we start parsing.
  • set parsedBody to null, incase parser.parser returns false.

That will cover all the cases.
<body></body></dummy> - will emit error and set parsedBody to nulll.
<body></body><dummy>- wont be completely parsed and parsedBody will remain null.

@dhruvbird
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about the case when the parser's internal buffer is buffering some input from before, which is what I am more worried about - since that data can corrupt future requests.

@satyamshekhar
Copy link
Collaborator

Choose a reason for hiding this comment

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

That will never happen.. dummy tag has to be closed.. which we close.. the content inside the dummy tag has to be valid.. for it to be closed properly.. closing the dummy tag before will lead to error.. opening another tag will leave it open..

The only case left is the top level CDATA(inside starting element) node.. which we completely ignore..

There arent any other cases left. (I think).

@dhruvbird
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it - it seems like you've covered all the interesting cases. I'll push this and let you know. Need help with the parser reset API instead of constructing a new parser on parsing failure.

@dhruvbird
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@satyamshekhar Can you verify if this looks okay to you? 3be0223

Please sign in to comment.