Check against integer overflow in RCTNetworking decodeTextData #16286
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
matthargett
approved these changes
Oct 11, 2017
cdlewis
commented
Oct 11, 2017
Libraries/Network/RCTNetworking.mm
Outdated
| @@ -408,7 +408,7 @@ + (NSString *)decodeTextData:(NSData *)data fromResponse:(NSURLResponse *)respon | |||
| NSData *newCarryData = [currentCarryData subdataWithRange:NSMakeRange(encodedResponseLength, currentCarryData.length - encodedResponseLength)]; | |||
| [inputCarryData setData:newCarryData]; | |||
| } else { | |||
| [inputCarryData setData:nil]; | |||
| [inputCarryData setLength:0]; | |||
There was a problem hiding this comment.
I intended to have this in my original PR. You can't setData:nil.
|
Hey thanks for the PR, just some questions: What does happen after your changes when sending bad data? |
|
After my changes we parse the data without crashing. The result is the same as parsing it via XMLHttpRequest on the web. You can verify this by running the code from On Web: On iOS: On Android: |
janicduplessis
approved these changes
Oct 15, 2017
|
@facebook-github-bot shipit |
facebook-github-bot
added
the
Import Started
facebook-github-bot
approved these changes
Oct 15, 2017
There was a problem hiding this comment.
@janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
facebook-github-bot
added
Import Failed
and removed
Import Started
|
@janicduplessis looks like there were some issues merging this change with Facebook's internal repo? |
facebook-github-bot
added
the
Import Started
facebook-github-bot
approved these changes
Nov 7, 2017
There was a problem hiding this comment.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
cdlewis
added a commit
to cdlewis/react-native
that referenced
this pull request
Nov 19, 2017
Summary:
It's currently possible to crash React Native on iOS when using XMLHTTPRequest with onreadystatechange by having the server send a bunch of bad unicode (we found the problem when a bad deploy caused this to happen).
This is due to an integer overflow when handling carryover data in decodeTextData.
Create Express server with mock endpoint:
```js
var express = require('express');
var app = express();
app.get('/', function(req, res) {
res.writeHead(200, {'content-type': 'text/plain; charset=utf-8'});
res.flushHeaders();
res.write(new Buffer(Array(4097).join(0x48).concat(0xC2)));
res.write(new Buffer([0xA9]));
res.end();
});
app.listen(3000);
```
Create React Native application which tries to hit the endpoint:
```js
export default class App extends Component<{}> {
componentDidMount() {
const xhr = new XMLHttpRequest()
xhr.open('get', 'http://localhost:3000', true);
xhr.onreadystatechange = function () {
if(xhr.readyState === XMLHttpRequest.DONE && xhr.status === 200) {
console.warn(xhr.responseText);
}
};
xhr.send();
}
render() {
return null;
}
}
```
Observe that the application crashes when running master and doesn't when including the changes from this pull request.
[IOS] [BUGFIX] [RCTNetworking] - |Check against integer overflow when parsing response|
Closes facebook#16286
Differential Revision: D6060975
Pulled By: hramos
fbshipit-source-id: 650e401a3bc033725078ea064f8fbca5441f9db5
react-native-bot
removed
Import Started
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's currently possible to crash React Native on iOS when using XMLHTTPRequest with onreadystatechange by having the server send a bunch of bad unicode (we found the problem when a bad deploy caused this to happen).
This is due to an integer overflow when handling carryover data in decodeTextData.
Test Plan
Create Express server with mock endpoint:
Create React Native application which tries to hit the endpoint:
Observe that the application crashes when running master and doesn't when including the changes from this pull request.
Release Notes
[IOS] [BUGFIX] [RCTNetworking] - |Check against integer overflow when parsing response|