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

Signed integer overflow when SipHasher processes inputs >= 2 GB #19930

Closed
practicalswift opened this issue Sep 10, 2020 · 1 comment · Fixed by #19931
Closed

Signed integer overflow when SipHasher processes inputs >= 2 GB #19930

practicalswift opened this issue Sep 10, 2020 · 1 comment · Fixed by #19931
Labels

Comments

@practicalswift
Copy link
Contributor

Signed integer overflow when SipHasher processes inputs >= 2 GB.

Live demo:

$ src/test/fuzz/simplest_possible_siphash_fuzzer -rss_limit_mb=8000 crash-061a172add013c03beedf57eb2a121a8289696af
crypto/siphash.cpp:56:10: runtime error: signed integer overflow: 2147483647 + 1 cannot be represented in type 'int'
$ cat src/test/fuzz/simplest_possible_siphash_fuzzer.cpp
#include <cstdint>
#include <vector>

#include <crypto/siphash.h>

void test_one_input(const std::vector<uint8_t>& buffer)
{
    CSipHasher(0, 0).Write(buffer.data(), buffer.size()).Finalize();
}

Credits to @elichai who submitted a differential SipHasher fuzzer in #19920 and @guidovranken who first spotted the issue. Thanks!

Remember: don't trust -- fuzz! :)

@sipa
Copy link
Member

sipa commented Sep 10, 2020

Not strictly an issue in our codebase, as all uses of CSipHasher are limited in size, but it's trivial to fix: #19931.

Thanks!

sidhujag pushed a commit to syscoin/syscoin that referenced this issue Sep 15, 2020
812037c Change CSipHasher's count variable to uint8_t (Pieter Wuille)

Pull request description:

  SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), but only the low 8 bits of the length matter. Because of that we should use an unsigned type to track the length (as any signed type could overflow, which is UB). `uint8_t` is sufficient, however.

  Fixes bitcoin#19930.

ACKs for top commit:
  laanwj:
    anyhow re-ACK 812037c
  elichai:
    utACK 812037c
  practicalswift:
    ACK 812037c
  theStack:
    ACK 812037c

Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Sep 17, 2021
812037c Change CSipHasher's count variable to uint8_t (Pieter Wuille)

Pull request description:

  SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), but only the low 8 bits of the length matter. Because of that we should use an unsigned type to track the length (as any signed type could overflow, which is UB). `uint8_t` is sufficient, however.

  Fixes bitcoin#19930.

ACKs for top commit:
  laanwj:
    anyhow re-ACK 812037c
  elichai:
    utACK 812037c
  practicalswift:
    ACK 812037c
  theStack:
    ACK 812037c

Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this issue Sep 24, 2021
812037c Change CSipHasher's count variable to uint8_t (Pieter Wuille)

Pull request description:

  SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), but only the low 8 bits of the length matter. Because of that we should use an unsigned type to track the length (as any signed type could overflow, which is UB). `uint8_t` is sufficient, however.

  Fixes bitcoin#19930.

ACKs for top commit:
  laanwj:
    anyhow re-ACK 812037c
  elichai:
    utACK 812037c
  practicalswift:
    ACK 812037c
  theStack:
    ACK 812037c

Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
kwvg pushed a commit to kwvg/dash that referenced this issue Oct 12, 2021
812037c Change CSipHasher's count variable to uint8_t (Pieter Wuille)

Pull request description:

  SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), but only the low 8 bits of the length matter. Because of that we should use an unsigned type to track the length (as any signed type could overflow, which is UB). `uint8_t` is sufficient, however.

  Fixes bitcoin#19930.

ACKs for top commit:
  laanwj:
    anyhow re-ACK 812037c
  elichai:
    utACK 812037c
  practicalswift:
    ACK 812037c
  theStack:
    ACK 812037c

Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
gades pushed a commit to cosanta/cosanta-core that referenced this issue Nov 21, 2023
812037c Change CSipHasher's count variable to uint8_t (Pieter Wuille)

Pull request description:

  SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), but only the low 8 bits of the length matter. Because of that we should use an unsigned type to track the length (as any signed type could overflow, which is UB). `uint8_t` is sufficient, however.

  Fixes bitcoin#19930.

ACKs for top commit:
  laanwj:
    anyhow re-ACK 812037c
  elichai:
    utACK 812037c
  practicalswift:
    ACK 812037c
  theStack:
    ACK 812037c

Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
gades pushed a commit to piratecash/pirate that referenced this issue Dec 9, 2023
812037c Change CSipHasher's count variable to uint8_t (Pieter Wuille)

Pull request description:

  SipHash technically supports arbitrarily long inputs (at least, I couldn't find a limit in the [paper](https://eprint.iacr.org/2012/351.pdf)), but only the low 8 bits of the length matter. Because of that we should use an unsigned type to track the length (as any signed type could overflow, which is UB). `uint8_t` is sufficient, however.

  Fixes bitcoin#19930.

ACKs for top commit:
  laanwj:
    anyhow re-ACK 812037c
  elichai:
    utACK 812037c
  practicalswift:
    ACK 812037c
  theStack:
    ACK 812037c

Tree-SHA512: 5b1440c9e4591460da198991fb421ad47d2d96def2014e761726ce361aa9575752f2c4085656e7e9badee3660ff005cc76fbd1afe4848faefe4502f3412bd896
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@sipa @practicalswift and others