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

Bad Inputs cause unhandled exceptions and permanent hang #196

Closed
kulgg opened this issue Apr 10, 2021 · 5 comments · Fixed by #197
Closed

Bad Inputs cause unhandled exceptions and permanent hang #196

kulgg opened this issue Apr 10, 2021 · 5 comments · Fixed by #197
Assignees
Labels
bug The result of a coding Error
Milestone

Comments

@kulgg
Copy link
Contributor

kulgg commented Apr 10, 2021

Description
These bugs were gathered while Fuzzing the parsing function with Sharpfuzz and AFL.
On bad inputs an HL7Exception should be thrown but other exceptions occur on some inputs.

Exceptions and Inputs

  • ArgumentOutOfRange
    b""
  • System
    b"MSH|0|||||||00|||2.7"
  • NullReference
    b"MSHH0\r"
  • IndexOutOfRange
    b"MSH|0|||||||||||0"
  • Regex
    b"MSH|^\x01\0|||||0||ACK^\|||2.2^0\r0|0|2^V~\\r0|0|00\r0)0"
  • System
    b"MSH|0|0|0|0|0|0|0|20\x7f|0|0|2.7"
  • Permanent Hang
    b"MSH|^|||||||^A|||2.2\r^AA"

Environmental Details

  • OS: Ubuntu 20.04
  • Target Framework netstandard2.0, net35, probably across all
  • Version 3.0.0 preview 2

Additional context
The permanent hang would potentially be DoS exploitable depending on what context the library is used in.
I will create a pull request that fixes these bugs and adds them as tests.

@milkshakeuk
Copy link
Member

@JlKmn thanks for this I will look at it & your pull request when I get chance.

@milkshakeuk milkshakeuk added bug The result of a coding Error enhancement and removed bug The result of a coding Error enhancement labels Apr 10, 2021
@kulgg
Copy link
Contributor Author

kulgg commented Apr 10, 2021

@milkshakeuk Great, once it is reviewed and merged I have deeper bugs of later Fuzzing Runs and fixes for them. I didn't want to have too much in one pull request.

@milkshakeuk
Copy link
Member

@JlKmn, I am currently working on porting the newer PipeParser from hapi to fix certain issues caused by unexpected segments (probably fixes other things too).

The current PipeParser you have fuzzed will remain but be renamed LegacyPipeParser.

It's probably good to have these fixes in the LegacyPipeParser anyway but it might be useful if you could do the fuzzing again when the new PipeParser is in.

Is that something you could do?

@milkshakeuk
Copy link
Member

milkshakeuk commented Apr 10, 2021

@JlKmn I'm not 100% sure, I'm just writing the unit tests to finish the PR off.
Hopefully in the next day or so?

b"MSHH0\r"

whats the b for in front of your bad inputs?

@kulgg
Copy link
Contributor Author

kulgg commented Apr 10, 2021

It's just to indicate that its python byte string output

@milkshakeuk milkshakeuk added this to the v3.0.0.0 milestone Apr 12, 2021
@milkshakeuk milkshakeuk added bug The result of a coding Error and removed awaiting review labels Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The result of a coding Error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants