8 ms·

I've fuzzed a small part of the GitHub API (2020)

80 points by ludovicianul 4y ago

londons_explore 4y ago
I think the real conclusion here is... No substantial security issues were discovered. There are a few things github could change to better follow security best practices to help avoid future bugs.
- dropofwill 4y ago
  I read it more as an example of how to use CATS to do this sort of fuzzing.
  - anentropic 4y ago
    Yes, CATS looks great, hadn't heard of it before!
londons_explore 4y ago
Doing this sort of thing to a live webservice is a good way to get a perma-ban...
- ludovicianul 4y ago
  Author here. There are no major security issues and it's also compliant with the API rate limiting restrictions.
  - masklinn 4y ago
    Including the new secondary rate limits? They’re quite annoying.
  - bobleeswagger 4y ago
    It's a fine line, if Github wanted to I'm sure they could write up a convincing cease and desist. I think they spoke up about it because your article gives the impression that this sort of thing (fuzzing APIs) is okay, which I wouldn't exactly say is positive.
    - ludovicianul 4y ago
      True. This is why I run it only for one path. I also run it against Kubernetes API and some of the payloads resulted in timeouts which I submitted to the security team.
binarysneaker 4y ago
GitHub performs fuzzing as part of their testing, so I'm not surprised there aren't any significant findings. I am surprised this is on hackernews two years later though.
plebb0t 4y ago
Ah yes, ISO 8061, Alpine ski-bindings — Selection of release torques values. https://www.iso.org/standard/75609.html https://www.iso.org/standard/75609.html Nice article btw.
- rfdonnelly 4y ago
  I submitted a PR https://github.com/ludovicianul/ludovicianul.github.io/pull/4 https://github.com/ludovicianul/ludovicianul.github.io/pull/...
  - ludovicianul 4y ago
    Thanks!
- jwilk 4y ago
  Quite popular misspelling: https://codesearch.debian.net/search?q=%28%3Fi%29%5Cbiso%5B+-%5D%2A8061%5Cb https://codesearch.debian.net/search?q=%28%3Fi%29%5Cbiso%5B+...
deluarseo 4y ago
sigmonsays 4y ago
the results of the fuzzing is overall very meh, but CATS looks like an interesting tool.
dtech 4y ago
> The InvalidValuesInEnumsFieldsFuzzer will send invalid values in enum fields. It expects a validation error in return. The GitHub API does not seem to reject invalid values, but rather convert them to a default value and respond successfully to the request. This is in contradiction with the OWASP recommendation around strong input validation and data type enforcing. Doing this is incredibly good practice for compatibility, because otherwise you can never add or remove enum values because older clients will break on the unknown values. I also fail to see how it violates the recommendation, invalid data doesn't enter the system. Imagine all webservers and browsers would need to be updated for every new HTTP header or status code is specified or web pages don't work anymore...
- ludovicianul 4y ago
  As long as an API rejects invalid values if they are not part of an agreed list (but not enum), the same logic should apply for fields explicitly marked as enums. HTTP headers are not meant to be a limited set. As for the HTTP methods, it's a clear recommendation from OWASP to explicitly reject methods not supported by your API. It's like converting everything not known to POST...
aszen 4y ago
The cats tool looks interesting I'm curious how it handles fuzzing id columns, which would need a reference to another resource to actually test them. I would like a tool which can take insertion order of the api and start inserting things at random based on a spec to test it.
- ludovicianul 4y ago
  Can you please detail a bit? Currently, there is some support for resource correlation. Especially when fuzzing DELETE, it will identify a successful POST based on conventions and use that as an identifier. Also checking that DELETEd resources are not available anymore.
  - aszen 4y ago
    Say i have an order record, it needs company and product. The fuzzer should be smart enough to first generate company and product then use them to create orders. Similarly the fuzzer should be able to build a whole graph of the api data model and be able to execute actions on it in a particular order testing all the actions
brap 4y ago
Would any of these errors be present if they used GraphQL instead of REST? Obviously the ones with headers can still happen, but it looks like everything else is related to schema enforcement
- ludovicianul 4y ago
  It's only OpenAPI for now with limited support for request templates.
  - brap 4y ago
    I know, I'm wondering if switching to something like GraphQL or tRPC would solve many of these issues, which seem to be around violating the OpenAPI contract

80 points by ludovicianul 4y ago

I think the real conclusion here is... No substantial security issues were discovered. There are a few things github could change to better follow security best practices to help avoid future bugs.

dropofwill 4y ago
I read it more as an example of how to use CATS to do this sort of fuzzing.
- anentropic 4y ago
  Yes, CATS looks great, hadn't heard of it before!

londons_explore 4y ago

Doing this sort of thing to a live webservice is a good way to get a perma-ban...

ludovicianul 4y ago
Author here. There are no major security issues and it's also compliant with the API rate limiting restrictions.
- masklinn 4y ago
  Including the new secondary rate limits? They’re quite annoying.
- bobleeswagger 4y ago
  It's a fine line, if Github wanted to I'm sure they could write up a convincing cease and desist. I think they spoke up about it because your article gives the impression that this sort of thing (fuzzing APIs) is okay, which I wouldn't exactly say is positive.
  - ludovicianul 4y ago
    True. This is why I run it only for one path. I also run it against Kubernetes API and some of the payloads resulted in timeouts which I submitted to the security team.

binarysneaker 4y ago

GitHub performs fuzzing as part of their testing, so I'm not surprised there aren't any significant findings. I am surprised this is on hackernews two years later though.

plebb0t 4y ago

Ah yes, ISO 8061, Alpine ski-bindings — Selection of release torques values. https://www.iso.org/standard/75609.html https://www.iso.org/standard/75609.html Nice article btw.

rfdonnelly 4y ago
I submitted a PR https://github.com/ludovicianul/ludovicianul.github.io/pull/4 https://github.com/ludovicianul/ludovicianul.github.io/pull/...
- ludovicianul 4y ago
  Thanks!
jwilk 4y ago
Quite popular misspelling: https://codesearch.debian.net/search?q=%28%3Fi%29%5Cbiso%5B+-%5D%2A8061%5Cb https://codesearch.debian.net/search?q=%28%3Fi%29%5Cbiso%5B+...

deluarseo 4y ago

sigmonsays 4y ago

the results of the fuzzing is overall very meh, but CATS looks like an interesting tool.

dtech 4y ago

> The InvalidValuesInEnumsFieldsFuzzer will send invalid values in enum fields. It expects a validation error in return. The GitHub API does not seem to reject invalid values, but rather convert them to a default value and respond successfully to the request. This is in contradiction with the OWASP recommendation around strong input validation and data type enforcing. Doing this is incredibly good practice for compatibility, because otherwise you can never add or remove enum values because older clients will break on the unknown values. I also fail to see how it violates the recommendation, invalid data doesn't enter the system. Imagine all webservers and browsers would need to be updated for every new HTTP header or status code is specified or web pages don't work anymore...

ludovicianul 4y ago
As long as an API rejects invalid values if they are not part of an agreed list (but not enum), the same logic should apply for fields explicitly marked as enums. HTTP headers are not meant to be a limited set. As for the HTTP methods, it's a clear recommendation from OWASP to explicitly reject methods not supported by your API. It's like converting everything not known to POST...

aszen 4y ago

The cats tool looks interesting I'm curious how it handles fuzzing id columns, which would need a reference to another resource to actually test them. I would like a tool which can take insertion order of the api and start inserting things at random based on a spec to test it.

ludovicianul 4y ago
Can you please detail a bit? Currently, there is some support for resource correlation. Especially when fuzzing DELETE, it will identify a successful POST based on conventions and use that as an identifier. Also checking that DELETEd resources are not available anymore.
- aszen 4y ago
  Say i have an order record, it needs company and product. The fuzzer should be smart enough to first generate company and product then use them to create orders. Similarly the fuzzer should be able to build a whole graph of the api data model and be able to execute actions on it in a particular order testing all the actions

brap 4y ago

Would any of these errors be present if they used GraphQL instead of REST? Obviously the ones with headers can still happen, but it looks like everything else is related to schema enforcement

ludovicianul 4y ago
It's only OpenAPI for now with limited support for request templates.
- brap 4y ago
  I know, I'm wondering if switching to something like GraphQL or tRPC would solve many of these issues, which seem to be around violating the OpenAPI contract