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

Fix vulnerabilities CWE-113 (issue #4208 issue #4209) #4210

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

omordyk
Copy link
Contributor

@omordyk omordyk commented Dec 16, 2024

Description

Software places user-controlled input in HTTP headers. An attacker could inject line separators (CR/LF sequences) that could split the response message generated by the software into two messages. The second response is completely under the control of the attacker (intermediate web proxies may cache it), with could produce multiple conditions (web defacement, cache poisoning, cross-site scripting or page hijacking, see CWE-113 for full details). If software needs to generate HTTP headers depending on user-controlled input, such input should be properly neutralized (a white-list validation excluding CR/LF is recommended). Please note that cookies are received and sent in 'Cookie' header in HTTP messages, so if the software generates a Cookie from user input, the input should be properly validated as well.

issue #4208
issue #4209

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

make and make test

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings
  • I have tagged the reviewers in a comment below incase my pull request is ready for a review
  • I have signed the commit message to agree to Developer Certificate of Origin (DCO) (to certify that you wrote or otherwise have the right to submit your contribution to the project.) by adding "--signoff" to my git commit command.

@omordyk
Copy link
Contributor Author

omordyk commented Dec 18, 2024

Ran SAST(Contrast) and checked that vulnerabilities were fixed.

@omordyk
Copy link
Contributor Author

omordyk commented Jan 7, 2025

Hi guys. Pls check my PR.

"sort"
"sync"
"time"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you undo the changes in import? Same for api/api.go

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@omordyk Please see the above comment, the rest looks good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks

Comment on lines 335 to 336
if !isValidInput(input) {
http.Error(w, "Invalid input", http.StatusBadRequest)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the error message be more specific?
Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, Thanks.

@omordyk omordyk force-pushed the security_4208_4209 branch from 74b5207 to 61745d6 Compare January 9, 2025 09:03
@LiilyZhang
Copy link
Contributor

@omordyk Please combine your commits into 1 commit, thanks

Signed-off-by: Oleksandr Mordyk <[email protected]>
@omordyk omordyk force-pushed the security_4208_4209 branch from cac2bf0 to c2f60bd Compare January 9, 2025 14:28
@omordyk omordyk merged commit 3163bff into master Jan 9, 2025
3 checks passed
@LiilyZhang LiilyZhang deleted the security_4208_4209 branch January 9, 2025 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vulnerability CWE-113 api/api.go:140 Vulnerability CWE-113 agreementbot/api.go:325
3 participants