Tl;dr : This article is analysis of over 50 submissions for a JavaScript integer overflow challenge. Many submissions did not address the root cause. A range check on the input as well as arithmetic output using a right data type can eliminate the vulnerability.
There is a lot to learn from the way developers go after addressing a security vulnerability. This will help us to find if the security bug was well understood. A security vulnerability like integer overflow can be trivial to trigger, however it may not be simple to get addressed effectively.
In this write-up, I have analysed over 50 submissions for JavaScript Start Here.js challenge. This challenge was a vulnerable JavaScript program that was inspired by Boeing 787 shutdown bug. I will introduce the program, following with analysis of ineffective patches and end with review of two good security approaches.
The challenge is still available. If you wish to try it, you may want to skip reading the rest of this article.
The program
After cloning the repository, we build, run and test the program:
> make build
[i] Building the program
Sending build context to Docker daemon 81.92kB
Step 1/10 : FROM secdim/play-js:latest
latest: Pulling from secdim/play-js
# SNIP
Step 10/10 : CMD ["npm", "start"]
---> Running in e556097676bd
Removing intermediate container e556097676bd
---> 5572ddb7a54e
Successfully built 5572ddb7a54e
Successfully tagged secdim.lab.js:latest
[i] Done! Program is ready to run
Once the container is built, we can run the program:
> make run
start
nodemon app.js
[nodemon] 2.0.20
[nodemon] to restart at any time, enter `rs`
[nodemon] watching path(s): *.*
[nodemon] watching extensions: js,mjs,json
[nodemon] starting `node app.js`
Server is listening on port: 8080
> curl "localhost:8080/?amount=100"
100 does not require approval
> curl "localhost:8080/?amount=1001"
1001 requires approval
Each challenge comes with two sets of tests, usability tests (src/app.test.js) that tests if the program runs as expected, security tests (src/appSecurity.test.js) that tests for the security weaknesses. We run usability tests by running make test.
> make test
[i] Running functionality tests
> test
> jest --verbose "app.test.js"
PASS ./app.test.js
usability
âś“ Requst for 500, should return 200 (35 ms)
âś“ Requst for 1001, should return 400 (4 ms)
âś“ when request /status, should return 200 (2 ms)
Test Suites: 1 passed, 1 total
Tests: 3 passed, 3 total
Snapshots: 0 total
Time: 0.586 s
Ran all test suites matching /app.test.js/i.
npm timing command:run-script Completed in 1582ms
npm timing command:test Completed in 1586ms
npm verb exit 0
npm timing npm Completed in 1625ms
npm info ok
[i] Well done! All functionality tests have been passed
Usability tests should always pass. If they fail, the challenge fails. Nobody cares about an unusable program. Run make securitytest to run security tests.
> make securitytest
# SNIP
● security › Arithmetic result into Int32bit max value, should throw RangeError exception
expect.assertions(1)
Expected one assertion to be called but received zero assertion calls.
35 |
36 | it('Arithmetic result into Int32bit max value, should throw RangeError exception', async () => {
> 37 | expect.assertions(1);
| ^
38 | try {
39 | approval(-2147483648 - 10 - 1)
40 | } catch (ex) {
at Object.<anonymous> (appSecurity.test.js:37:16)
Test Suites: 1 failed, 1 total
Tests: 4 failed, 4 total
Snapshots: 0 total
Time: 0.583 s
Ran all test suites matching /appSecurity.test.js/i.
npm timing command:run-script Completed in 1488ms
npm timing command:test Completed in 1491ms
npm verb exit 1
npm timing npm Completed in 1529ms
npm verb code 1
[!] Oh, no! Program has security bug(s)! Try again
As expected all security tests failed due to security weaknesses of the program. These security tests can guide us in fixing the security weaknesses.
> curl "localhost:8080/?amount=1000000000000"
1000000000000 does not require approval
As we can see, when amount is very large, the program returns an unexpected result.
Our objective is to pass security tests while not failing usability tests.
The insecure/anti pattern
The following code snippet shows the vulnerable part of the program.
var approval = (value) => {
var threshold = 1000;
var surcharge = 10;
var amount = Int32Array.from([value],x=>parseInt(x+surcharge));
if (amount[0] >= threshold) {
return true;
};
return false;
};
The approval function receives a value from an untrusted source. The parseInt() parses the value + surcharge from string to integer. The result is stored in Int32Array. Int32Array is a TypedArray. It has a fixed range with minimum and maximum ranges of -2,147,483,648 to 2,147,483,647.
Since we do arithmetic on an untrusted value without performing range check, this anti-pattern can result into numeric or integer overflow vulnerability (read this article to learn why this vulnerability happens almost everywhere.)
Security tests for this challenge explicitly check for the integer overflow and a few other security edge cases. For example, the following test expects approve to return RangeError when input is beyond Int32 range:
it('Int32bit maximum amount, should throw RangeError exception', async () => {
expect.assertions(1);
try {
approval(2147483648)
} catch (ex) {
expect(ex).toBeInstanceOf(RangeError);
}
})
Or the following test, checks if result of arithmetic, value + surcharge can go beyond int32 range and results in integer overflow:
it('Arithmetic result into Int32bit max value, should throw RangeError exception', async () => {
expect.assertions(1);
try {
approval(2147483647 - 10 + 1)
} catch (ex) {
expect(ex).toBeInstanceOf(RangeError);
}
});
We should handle both these conditions in our security patch.
How NOT to prevent integer overflow in JavaScript
Let’s look at some incorrect or inadequate approaches that have been submitted for this challenge. What is wrong with the following patch?
var approval = (value) => {
var threshold = 1000;
var surcharge = 10;
var amount = Int32Array.from([value],x=>parseInt(x));
if (amount[0] >= 2147483647 - 10 - 1) {
throw new RangeError("The value is too big.");
}
// We don't need to go all the way to min int here.
if (amount[0] < 0) {
throw new RangeError("The value is too small.");
}
if (amount[0] + surcharge >= threshold) {
return true;
};
return false;
};
The if (amount[0] >= 2147483647 - 10 - 1) check is ineffective. if the value is beyond int32 range, it will overflow when arithmetic is performed and the check is done on an already overflowed value.
Let’s look at another submission.
var approval = (value) => {
var threashold = 1000;
var surcharge = 10;
var amount = Int32Array.from([value],x=>parseInt(x+surcharge));
if (amount[0] - surcharge <= 0 || amount[0] + surcharge > 2147483647) {
throw new RangeError;
}
if (amount[0] >= threashold) {
return true;
};
return false;
};
Similar to above, if (amount[0] - surcharge <= 0 || amount[0] + surcharge > 2147483647) check is ineffective because it checks the output after storing it in a int32 type. Therefore, if value is very large, the check is performed on an already overflowed value and misses the security bug.
Now, what do you think is wrong with the following security patch?
const INT_MAX = ~(1 << 31);
var approval = (value) => {
if(value < 0) {
throw new RangeError();
}
if(value >= INT_MAX) {
throw new RangeError();
}
var threashold = 1000;
var surcharge = 10
const amount = value + surcharge;
if (amount >= INT_MAX) {
throw new RangeError();
}
if (amount >= threashold) {
return true;
};
return false;
};
This solution seems correct, but it has some subtle weaknesses. If we do not specify the data type, by default JavaScript uses Number data type. This type is based on IEEE 754 Double Precision and has a fixed range. Which means if a value is beyond a certain value (±9,007,199,254,740,991), the result is approximate (See Float Overflow in JavaScript).
Another issue is that by removing Int32Array, we lost its benefit. This patch has performance and efficiency issues that impacts usability of the program. Perhaps a better usability test could prevent this.
Two good approaches to prevent integer overflow in JavaScript
The following patch uses a smart method to check for integer overflow:
var approval = (value) => {
var threashold = 1000;
var surcharge = 10;
var amount = Int32Array.from([value],x=>parseInt(x+surcharge));
if((amount[0] < value && amount[0] < surcharge) || value < 0) {
throw new RangeError("Numer is out of range");
}
if (amount[0] >= threashold) {
return true;
};
return false;
};
- It does not introduce a redundant arithmetic to check for integer overflow.
- It has kept the benefits of
Int32Array - It uses an invariant or property that always remains true when adding two values.
Given two positive numbers, x and y, if x + y = z then z is always bigger or equal to x or y. This is the addition property and it always holds true.
In the patch, the amount[0] < value && amount[0] < surcharge examines this property. amount[0] cannot be smaller than value or surcharge unless an overflow happens.
This approach, let’s called it Post condition testing, is more advanced than the techniques we covered in JavaScript Secure Programming Fundamental I:
- Precondition testing: check the input before each arithmetic operation. If safe, proceed with the operation, otherwise throw an exception.
- Upcasting: use a larger datatype to perform the arithmetic. Check for an overflow within a smaller datatype’s range before downcasting.
- BigInteger: use a datatype that makes use of dynamic memory allocation.
Another approach to address integer overflow bug is as follow. It utilises the BigInt, a dynamically allocated datatype that is not restricted by a range. :
var approval = (value) => {
var threashold = 1000;
var surcharge = 10;
var converted_amount = BigInt(value) + BigInt(surcharge)
if (converted_amount < 0) {
throw new RangeError;
}
var amount = Int32Array.from([value], x => parseInt(x + surcharge));
if (amount[0] < 0) {
throw new RangeError;
}
if (amount[0] >= threashold) {
return true;
};
return false;
};
As discussed in this article, the benefit of using a dynamically allocated data type is that we would not need to worry about range restriction and it is safer to use.
Lessons learnt
A review of 50 submission for JavaScript Start Here.js challenge showed that the majority of submission don’t address the root cause or may overlook it.
To address numeric or integer overflow, we can use precondition testing, upcasting, dynamically allocated data type or post-condition testing.
A simple but powerful security check is range check. Make sure you apply a range check on all numeric data types, before and after any arithmetic operations. For more information see JavaScript Secure Programming Fundamental II.
Finally, any time you handle number values in JavaScript, it uses Number type and it requires safe range check.
That’s all.
If you would like to skill up in secure programming or code-assist penetration testing:
- This holiday season, we run a free and open
7x7 secure programming competition. This is great opportunity to skill up in secure programming. - Every month we host an online security programming game. It is open and free to join. Find our more at SecGames Meetup.