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

Add length check to github signature #187

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AdamKorcz
Copy link

Adds a length check to avoid an out-of-range panic.

@coveralls
Copy link

coveralls commented Nov 18, 2023

Coverage Status

coverage: 88.586% (-0.2%) from 88.765%
when pulling f4db242 on AdamKorcz:fix-out-of-range
into 53694f8 on go-playground:master.

@AdamKorcz AdamKorcz force-pushed the fix-out-of-range branch 3 times, most recently from 5554ed1 to 316ebee Compare November 18, 2023 15:26
@davidhadas
Copy link

@robinlieb, this PR relates to a Security Audit done to the CNCF Knative Project,
Knative uses this repository as a dependency and the audit requires this to be fixed.

Can you check to see if this PR is expected to be fixed any time soon, or should Knative look for alternatives instead?

@robinlieb
Copy link
Member

Hi @AdamKorcz,
is this change still needed after the merge of #173?

@AdamKorcz
Copy link
Author

Hi @AdamKorcz, is this change still needed after the merge of #173?

Yes

@AdamKorcz
Copy link
Author

@robinlieb Can you have a look at https://github.com/go-playground/webhooks/security/advisories/GHSA-m7vc-6h95-xrjq and invite @davidhadas as a collaborator there as well?

@evankanderson
Copy link

@robinlieb Can you have a look at https://github.com/go-playground/webhooks/security/advisories/GHSA-m7vc-6h95-xrjq and invite @davidhadas as a collaborator there as well?

I'm the other security lead on Knative, and I'd appreciate if I could be on this issue as well.

@evankanderson
Copy link

Actually, #173 did fix this by swapping []byte(signature[5:]) for strings.TrimSignature, which removes the slice indexing.

I think we can close this given #173 being merged. Do you agree @AdamKorcz ?

@robinlieb
Copy link
Member

Seems like I don't have access neither. @deankarn can you have a look and add me and the two mentioned in the thread to this security advisory?

@deankarn
Copy link
Collaborator

deankarn commented Dec 6, 2023

Maybe I missed something how would I add someone to the security advisory? Not sure I understand.

@AdamKorcz
Copy link
Author

Maybe I missed something how would I add someone to the security advisory? Not sure I understand.

https://docs.github.com/en/code-security/security-advisories/working-with-repository-security-advisories/adding-a-collaborator-to-a-repository-security-advisory

@deankarn
Copy link
Collaborator

deankarn commented Dec 7, 2023

OK y'all should be added now as collaborators.

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.

6 participants