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

Prevent memory access errors in many native calls by adding checks #95

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

timmc
Copy link
Contributor

@timmc timmc commented Feb 8, 2021

This is not comprehensive coverage, but should take care of a number of
unguarded calls. It comes out of issue #81 in which a call to
cryptoSecretStreamPush with an incorrectly sized array lead to a VM
crash.

  • Add array checkers to BaseChecker
  • Simplify some existing checks by moving the throw into the check
    method; add reporting of which variable failed a check
  • Add checkers for SecretStream
  • Split PwHash.Checker.checkAll into individual check calls to reduce
    chance of positional argument errors

This is not comprehensive coverage, but should take care of a number of
unguarded calls. It comes out of issue terl#81 in which a call to
`cryptoSecretStreamPush` with an incorrectly sized array lead to a VM
crash.

- Add array checkers to BaseChecker
- Simplify some existing checks by moving the `throw` into the check
  method; add reporting of which variable failed a check
- Add checkers for SecretStream
- Split PwHash.Checker.checkAll into individual check calls to reduce
  chance of positional argument errors
@timmc
Copy link
Contributor Author

timmc commented Feb 8, 2021

Tests fail on a call to sodiumPad, but I think the test may be wrong: https://github.com/terl/lazysodium-java/blob/f7f0025/src/test/java/com/goterl/lazycode/lazysodium/PaddingTest.java#L19-L23

Shouldn't the max buffer length always be less than or equal to the length of the buffer?

@ionspin
Copy link
Contributor

ionspin commented Feb 21, 2021

@timmc I haven't checked in a while, but I think sodiumPad is still broken, details are in #85

@timmc
Copy link
Contributor Author

timmc commented Apr 16, 2021

For reference, the ./gradlew test currently produces this test failure:

java.lang.IllegalArgumentException: Provided buffer array length is larger than array
	at com.goterl.lazysodium.utils.BaseChecker.checkArrayLength(BaseChecker.java:59)
	at com.goterl.lazysodium.utils.BaseChecker.checkArrayLength(BaseChecker.java:50)
	at com.goterl.lazysodium.LazySodium.sodiumPad(LazySodium.java:186)
	at com.goterl.lazysodium.PaddingTest.pad(PaddingTest.java:22)

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