Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix arithmetic overflow on untrusted input #2

Merged
merged 2 commits into from
Nov 26, 2023

Conversation

gh123man
Copy link
Contributor

@gh123man gh123man commented Nov 26, 2023

I reported a similar bug to the original hashIds repo (that was never addressed). I discovered this repo and decided to test the same issue and it also exists here.

In it's current state, you cannot safely decode arbitrary untrusted user input (Imagine a user mistyping a url - which is how I originally discovered this in my app telemetry). I added a test case to this PR to demonstrate the problem and validate the fix. Here is an example:

func testDecodeOfUntrustedInput() throws {
        let sqids = Sqids()
        let badInput = try sqids.encode([Int64.max]) + "a" // Some untrusted user input
        _ = try sqids.decode(badInput) // 💥 Crash
    }
    

By generating a squid of Int64.max and appending an a to it, you create an id that when decoded overflows an Int64. In swift, this will trigger an arithmetic overflow that will crash your program. It is impossible to validate a valid sqid before trying to decode it, so there is no valid way to sanitize user input which risks crashing your program.

There are two ways to fix this:

  1. allow overflow (such that the ID is decoded to an incorrect Int
  2. throw an error when overflow is detected.

This PR uses approach 2 because the decode function already throws, so it doesn't change the overall usability. (in the original hashids library, this required a bigger (api breaking change) because the original decode function did not throw.

Solution 1 is also an option but will produce incorrect results. You can see that implementation in: cc4be61

@gh123man gh123man changed the title Fix arithmetic overflow on Untrusted input Fix arithmetic overflow on untrusted input Nov 26, 2023
@macmoonshine macmoonshine merged commit 3c24dbd into sqids:main Nov 26, 2023
1 check passed
@macmoonshine
Copy link
Collaborator

macmoonshine commented Nov 26, 2023

Hey Brian,
thank you for your pull request. I have applied the changes to the main branch. I added an XCTFail() to the test so that the absence of the exception is also evaluated as an error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants