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.

Sign up for GitHub

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

Jump to bottom

Issue #316 Fixing issues in XSSInImgTagAttribute Vulnerability #366

Merged

preetkaran20 merged 11 commits into SasanLabs:master from jpralle:issue316_fixXssInImgTagAttribute

Apr 20, 2022

Contributor

jpralle commented Mar 25, 2022

Renamed class and reworked secure implementation to actually validate the input and not just use a simple allow list. Also added a unit test for the secure implementation.

This is our first open source contribution, we hope we did it in the right manner. If not, please provide some help.

jpralle added 3 commits

March 25, 2022 14:57


          Change class name to match vulnerability type (issue 316)

0ffcc30


          Rework secure level to actually validate for malicious inout values (…

58a216b

…issue 316)


          Reformatted code

938abc4

jpralle mentioned this pull request

Fixing issues in XSSInImgTagAttribute Vulnerability #316

Closed

preetkaran20 reviewed

View reviewed changes

...asanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttributeInjectionVulnerability.java Outdated Show resolved Hide resolved

preetkaran20 reviewed

View reviewed changes

...asanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttributeInjectionVulnerability.java Outdated Show resolved Hide resolved

preetkaran20 reviewed

View reviewed changes

...asanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttributeInjectionVulnerability.java Outdated Show resolved Hide resolved

preetkaran20 reviewed

View reviewed changes

...asanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttributeInjectionVulnerability.java Outdated Show resolved Hide resolved

preetkaran20 reviewed

View reviewed changes

...labs/service/vulnerability/xss/reflected/XSSInImgTagAttributeInjectionVulnerabilityTest.java Outdated Show resolved Hide resolved

preetkaran20 reviewed

View reviewed changes

...labs/service/vulnerability/xss/reflected/XSSInImgTagAttributeInjectionVulnerabilityTest.java Outdated Show resolved Hide resolved

preetkaran20 reviewed

View reviewed changes

...labs/service/vulnerability/xss/reflected/XSSInImgTagAttributeInjectionVulnerabilityTest.java Outdated Show resolved Hide resolved

preetkaran20 requested changes

View reviewed changes

Member

preetkaran20 left a comment

Added few comments. Please have a look.

thanks,
Karan


          Issue SasanLabs#316 fixed code remarks

656ca2b

- renamed class and test
- corrected levels
- removed path traversal
- changed RequestParam from map to single param
- added input validation to level 7

preetkaran20 reviewed

View reviewed changes

src/main/java/org/sasanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttribute.java Show resolved Hide resolved

preetkaran20 reviewed

View reviewed changes

src/main/java/org/sasanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttribute.java Outdated Show resolved Hide resolved

preetkaran20 reviewed

View reviewed changes

Member

preetkaran20 left a comment

Overall, it LGTM, just few small questions.

preetkaran20 reviewed

View reviewed changes

src/test/java/org/sasanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttributeTest.java Show resolved Hide resolved


          Issue SasanLabs#316 Added allowed values check to secure implementation

209573a

preetkaran20 reviewed

View reviewed changes

src/main/java/org/sasanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttribute.java Outdated Show resolved Hide resolved


          Issue SasanLabs#316: Reformat code with spotless apply

ff1ca10

Member

preetkaran20 commented Apr 19, 2022

jpralle added 2 commits

April 20, 2022 08:28


          Issue SasanLabs#316 Add author information

45b4faa


          Issue SasanLabs#316 Remove null byte check, as we don't have it in Java

98e5e33

Contributor Author

jpralle commented Apr 20, 2022

We should be all done now with your comments. Thanks for the review and all the hints and tipps :)

preetkaran20 reviewed

View reviewed changes

src/main/java/org/sasanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttribute.java Outdated Show resolved Hide resolved

preetkaran20 reviewed

View reviewed changes

src/main/java/org/sasanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttribute.java Outdated Show resolved Hide resolved

jpralle added 2 commits

April 20, 2022 10:52


          Issue SasanLabs#316 Now really remove null byte check, accidently rem…

7f001b8

…oved the wrong check before


          Issue SasanLabs#316 Add allowed values check to level 6

94b626e

preetkaran20 reviewed

View reviewed changes

src/main/java/org/sasanlabs/service/vulnerability/xss/reflected/XSSInImgTagAttribute.java

-                          }
+                      if ((imageLocation.startsWith(IMAGE_RESOURCE_PATH)
+                                      && imageLocation.endsWith(FILE_EXTENSION))
+                              || allowedValues.contains(imageLocation)) {

Member

preetkaran20 Apr 20, 2022

|| allowedValues.contains(imageLocation)

this check is also not required. and also please run ./gradlew spotlessApply

Contributor Author

jpralle Apr 20, 2022

But before you explicitly asked me to add that part. I guess, you lost me a bit, sorry.

Member

preetkaran20 Apr 20, 2022

aah no, i think there is a confusion, i asked for level6 not level7 as level7 is added by you.

Member

preetkaran20 Apr 20, 2022

you can even left this condition as well. please run ./gradlew spotlessApply and we can merge this PR. thanks

preetkaran20 approved these changes

View reviewed changes

Member

preetkaran20 left a comment

|| allowedValues.contains(imageLocation)

this check is also not required. and also please run ./gradlew spotlessApply


          Issue SasanLabs#316 gradle spotless apply

649eed8

Contributor Author

jpralle commented Apr 20, 2022

|| allowedValues.contains(imageLocation)
this check is also not required. and also please run ./gradlew spotlessApply

Did so. Sorry, your code style differs a bit from what I'm used to.

Member

preetkaran20 commented Apr 20, 2022

Hi @jpralle and other,

Thanks a lot for the changes and for persistently looking into the comments. LGTM, merging it.

thanks,
Karan

preetkaran20 merged commit 0d3e0f3 into SasanLabs:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet