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

XSS Security Implementation and Ongoing SQL Injection Hardening #481

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions .classpath
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
<?xml version="1.0" encoding="UTF-8"?>
<classpath>
<classpathentry kind="src" output="bin/main" path="src/main/java">
<classpathentry kind="src" output="bin/test" path="src/test/java">
<attributes>
<attribute name="gradle_scope" value="test"/>
<attribute name="gradle_used_by_scope" value="test"/>
<attribute name="test" value="true"/>
</attributes>
</classpathentry>
<classpathentry kind="src" output="bin/main" path="src/main/resources">
<attributes>
<attribute name="gradle_scope" value="main"/>
<attribute name="gradle_used_by_scope" value="main,test"/>
</attributes>
</classpathentry>
<classpathentry kind="src" output="bin/main" path="src/main/resources">
<classpathentry kind="src" output="bin/main" path="src/main/java">
<attributes>
<attribute name="gradle_scope" value="main"/>
<attribute name="gradle_used_by_scope" value="main,test"/>
Expand Down
11 changes: 11 additions & 0 deletions .project
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,15 @@
<nature>org.eclipse.jdt.core.javanature</nature>
<nature>org.eclipse.buildship.core.gradleprojectnature</nature>
</natures>
<filteredResources>
<filter>
<id>1738769978762</id>
<name></name>
<type>30</type>
<matcher>
<id>org.eclipse.core.resources.regexFilterMatcher</id>
<arguments>node_modules|\.git|__CREATED_BY_JAVA_LANGUAGE_SERVER__</arguments>
</matcher>
</filter>
</filteredResources>
</projectDescription>
11 changes: 11 additions & 0 deletions .settings/org.eclipse.buildship.core.prefs
Original file line number Diff line number Diff line change
@@ -1,2 +1,13 @@
arguments=--init-script /home/lyes/.config/Code/User/globalStorage/redhat.java/1.39.0/config_linux/org.eclipse.osgi/58/0/.cp/gradle/init/init.gradle --init-script /home/lyes/.config/Code/User/globalStorage/redhat.java/1.39.0/config_linux/org.eclipse.osgi/58/0/.cp/gradle/protobuf/init.gradle
auto.sync=false
build.scans.enabled=false
connection.gradle.distribution=GRADLE_DISTRIBUTION(WRAPPER)
connection.project.dir=
eclipse.preferences.version=1
gradle.user.home=
java.home=/usr/lib/jvm/jdk-23.0.1-oracle-x64
jvm.arguments=
offline.mode=false
override.workspace.settings=true
show.console.view=true
show.executions.view=true
5 changes: 5 additions & 0 deletions .settings/org.eclipse.jdt.core.prefs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
eclipse.preferences.version=1
org.eclipse.jdt.core.classpath.outputOverlappingAnotherSource=ignore
org.eclipse.jdt.core.compiler.codegen.targetPlatform=1.8
org.eclipse.jdt.core.compiler.compliance=1.8
org.eclipse.jdt.core.compiler.source=1.8
3 changes: 3 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"java.configuration.updateBuildConfiguration": "interactive"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package org.sasanlabs.service.vulnerability.sampleVulnerability;
Copy link
Member

Choose a reason for hiding this comment

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

can you please remove SampleVulnerability related classes. Add them to .gitignore as well.

Copy link
Author

Choose a reason for hiding this comment

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

Yes i'll try to do it.


import org.sasanlabs.internal.utility.LevelConstants;
import org.sasanlabs.internal.utility.Variant;
import org.sasanlabs.internal.utility.annotations.AttackVector;
import org.sasanlabs.internal.utility.annotations.VulnerableAppRequestMapping;
import org.sasanlabs.internal.utility.annotations.VulnerableAppRestController;
import org.sasanlabs.service.vulnerability.bean.GenericVulnerabilityResponseBean;
import org.sasanlabs.vulnerability.types.VulnerabilityType;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.RequestParam;

/**
* This is a sample vulnerability for helping developers in adding a new Vulnerability for
* VulnerableApp
*
* @author KSASAN [email protected]
*/
/**
* {@code VulnerableAppRestController} annotation is similar to {@link
* org.springframework.stereotype.Controller} Annotation
*/
@VulnerableAppRestController(
/**
* "descriptionLabel" parameter of annotation is i18n label stored in {@link
* /VulnerableApp/src/main/resources/i18n/}. This descriptionLabel
* will be shown in the UI as the description of the Vulnerability. It helps students to
* learn about the vulnerability and can also include some of the useful references etc.
*/
descriptionLabel = "SAMPLE_VULNERABILITY",
/**
* "value" parameter of annotation is used to create the request mapping. e.g. for the below
* parameter value, /VulnerableApp/SampleVulnerability will be created as URI Path.
*/
value = "SampleVulnerability")
public class SampleVulnerability {

/**
* {@code AttackVector} annotation is used to create the Hints section in the User Interface.
* This annotation can be mentioned multiple times in case the same vulnerability level
*/
@AttackVector(
/**
* "vulnerabilityExposed" parameter is used to depict the Vulnerability exposed by the
* level. For example say a level is exposing SQL_INJECTION.
*/
vulnerabilityExposed = VulnerabilityType.SAMPLE_VULNERABILITY,
/**
* "description" parameter of annotation is i18n label stored in {@link
* /VulnerableApp/src/main/resources/i18n/}. This description
* will be shown in the UI as hint to give some indication on how the level is handling
* input to help user to crack the level.
*/
description = "SAMPLE_VULNERABILITY_USER_INPUT_HANDLING_INJECTION",

/**
* "payload" parameter of annotation is i18n label stored in {@link
* /VulnerableApp/src/main/resources/attackvectors/*.properties}. This payload will be
* shown in UI to help users find/exploit the vulnerability
*/
payload = "NOT_APPLICABLE")
/**
* This annotation is similar to {@link RequestMapping} SpringBoot annotation. It will map the
* endpoint to /VulnerableApp/SampleVulnerability/LEVEL_1 where LEVEL_1 is coming from the value
* parameter.
*/
@VulnerableAppRequestMapping(
/**
* "value" parameter is used to map the level to URI path
* /VulnerableApp/SampleVulnerability/${value}.
*/
value = LevelConstants.LEVEL_1,

/**
* "htmlTemplate" is used to load the UI for the level for taking input from the user.
* It points to files in directory
* src/main/resource/static/templates/${VulnerabilityName} e.g.
* src/main/resource/static/templates/SampleVulnerability as ${htmlTemplate}.js,
* ${htmlTemplate}.css, ${htmlTemplate}.html. e.g. in this case it will be:
* src/main/resource/static/templates/SampleVulnerability/LEVEL_1/SampleVulnerability_Level1.js
* etc
*
* <p>CSS, JS and HTML are all loaded to render the UI.
*/
htmlTemplate = "LEVEL_1/SampleVulnerability")
public GenericVulnerabilityResponseBean<String> sampleUnsecuredLevel(@RequestParam("name") String key) {
/** Add Business logic here */
return new GenericVulnerabilityResponseBean<>("Not Implemented", true);
}

/** For secured level there is no need for {@link AttackVector} annotation. */
@VulnerableAppRequestMapping(
value = LevelConstants.LEVEL_2,

// Can reuse the same UI template in case it doesn't change between levels
htmlTemplate = "LEVEL_1/SampleVulnerability",
/**
* "variant" parameter defines whether the level is secure or not and same is depicted
* in the UI as a closed lock and open lock icon. Default value of the variant is
* UNSECURE so in case a secure level is added, please add the variant as {@link
* Variant#SECURE}
*/
variant = Variant.SECURE)
public GenericVulnerabilityResponseBean<String> sampleSecuredLevel(@RequestParam("name") String key) {
/** Add Business logic here */
return new GenericVulnerabilityResponseBean<>("Not Implemented", true);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -218,4 +218,54 @@ public ResponseEntity<String> getVulnerablePayloadLevel7(
post -> StringEscapeUtils.escapeHtml4(post)),
HttpStatus.OK);
}
//escape html and delete all the script and img tags
@VulnerableAppRequestMapping(
value = LevelConstants.LEVEL_8,
htmlTemplate = "LEVEL_1/PersistentXSS",
variant = Variant.SECURE)
public ResponseEntity<String> getSecurePayloadLevel8(
@RequestParam Map<String, String> queryParams) {
return new ResponseEntity<>(
this.getCommentsPayload(
queryParams,
LevelConstants.LEVEL_8,
post -> StringEscapeUtils.escapeHtml4(
post.replaceAll("(?i)<script.*?>.*?</script>", "")
Copy link
Member

Choose a reason for hiding this comment

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

With escapeHtml4., do we even need escaping of script tag?

Copy link
Author

Choose a reason for hiding this comment

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

I mean there is some flaws i found, for example you can trigger an xss if the charset is explicitly set to UTF-7 you can use a payload like this one :
+ADw-script+AD4-alert(document.location)+ADw-/script+AD4-

Let me think what do you think

.replaceAll("(?i)<img.*?>", ""))),
HttpStatus.OK);
}
//delete all the html tags
@VulnerableAppRequestMapping(
value = LevelConstants.LEVEL_9,
htmlTemplate = "LEVEL_1/PersistentXSS",
variant = Variant.SECURE)
public ResponseEntity<String> getSecurePayloadLevel9(
@RequestParam Map<String, String> queryParams) {
Function<String, String> function =
(post) -> {
String sanitizedPost = post.replaceAll("<.*?>", ""); // Delete all the html balises
Copy link
Member

Choose a reason for hiding this comment

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

with html4 escape, do we need replaceall?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah you are right, we dont actually need that replace.

return StringEscapeUtils.escapeHtml4(sanitizedPost);
};
return new ResponseEntity<>(
this.getCommentsPayload(queryParams, LevelConstants.LEVEL_9, function),
HttpStatus.OK);
}
//delete all the js calls
@VulnerableAppRequestMapping(
value = LevelConstants.LEVEL_10,
htmlTemplate = "LEVEL_1/PersistentXSS",
variant = Variant.SECURE)
public ResponseEntity<String> getSecurePayloadLevel10(
@RequestParam Map<String, String> queryParams) {
Function<String, String> function =
(post) -> {
String sanitizedPost = StringEscapeUtils.escapeHtml4(post);
sanitizedPost = sanitizedPost.replaceAll("(?i)javascript:", ""); // Delete all the js calls
return StringEscapeUtils.escapeHtml4(sanitizedPost);
};
return new ResponseEntity<>(
this.getCommentsPayload(queryParams, LevelConstants.LEVEL_10, function),
HttpStatus.OK);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,15 @@
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.util.HtmlUtils;
import org.springframework.http.HttpHeaders;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.io.File;
import java.nio.file.Files;
import java.io.IOException;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.net.URLConnection;

/**
* This class contains XSS vulnerabilities which are present in Image Tag attribute.
Expand Down Expand Up @@ -201,4 +210,88 @@ public ResponseEntity<String> getVulnerablePayloadLevelSecure(
return new ResponseEntity<>(HttpStatus.BAD_REQUEST);
}
}

// Level 8: Protection avec Content Security Policy (CSP)
@AttackVector(
vulnerabilityExposed = VulnerabilityType.REFLECTED_XSS,
description = "XSS_CSP_PROTECTION")
@VulnerableAppRequestMapping(
value = LevelConstants.LEVEL_8,
variant = Variant.SECURE,
htmlTemplate = "LEVEL_1/XSS")
public ResponseEntity<String> getVulnerablePayloadLevelSecure2(@RequestParam(PARAMETER_NAME) String imageLocation) {
HttpHeaders headers = new HttpHeaders();
headers.add("Content-Security-Policy", "default-src 'self'; img-src 'self'");
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add a level which is insecure where csp header is lenient if already that level is not there.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'll add an insecure level with a weak CSP.

String vulnerablePayloadWithPlaceHolder = "<img src=\"%s\" width=\"400\" height=\"300\"/>";
String payload = String.format(vulnerablePayloadWithPlaceHolder, HtmlUtils.htmlEscape(imageLocation));
return new ResponseEntity<>(payload, headers, HttpStatus.OK);
}

// Level 9: Protection avec validation du type MIME
@AttackVector(
vulnerabilityExposed = VulnerabilityType.REFLECTED_XSS,
description = "XSS_MIME_TYPE_VALIDATION")
@VulnerableAppRequestMapping(
value = LevelConstants.LEVEL_9,
variant = Variant.SECURE,
Copy link
Member

Choose a reason for hiding this comment

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

are we sure that this cannot be broken?

Copy link
Author

Choose a reason for hiding this comment

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

Requesting a MIME type in the backend cannot be bypassed. If the user submits anything other than an image, the requested resource does not appear.
In the other hand if the user submit something like a file, yes the mime type can be bypassed.

htmlTemplate = "LEVEL_1/XSS")
public ResponseEntity<String> validateMimeType(@RequestParam(PARAMETER_NAME) String imageLocation) {
Path filePath = Paths.get(imageLocation);
try {
File file = filePath.toFile();
String mimeType = Files.probeContentType(filePath);
if (mimeType == null) {
mimeType = URLConnection.guessContentTypeFromName(file.getName());
}
if (mimeType != null && mimeType.startsWith("image/")) {
String vulnerablePayloadWithPlaceHolder = "<img src=\"%s\" width=\"400\" height=\"300\"/>";
Copy link
Member

Choose a reason for hiding this comment

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

better to create a string constant for this line as it is being used multiple times in all the vulnerability levels.

Copy link
Author

Choose a reason for hiding this comment

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

Noted !

String payload = String.format(vulnerablePayloadWithPlaceHolder, imageLocation);
return new ResponseEntity<>(payload, HttpStatus.OK);
}
} catch (IOException e) {
return new ResponseEntity<>("Error detecting MIME Type", HttpStatus.INTERNAL_SERVER_ERROR);
}
return new ResponseEntity<>("Invalid MIME Type", HttpStatus.BAD_REQUEST);
}

// Level 10: Protection avec hachage du chemin de l'image
@AttackVector(
vulnerabilityExposed = VulnerabilityType.REFLECTED_XSS,
description = "XSS_HASH_VALIDATION")
@VulnerableAppRequestMapping(
value = LevelConstants.LEVEL_10,
variant = Variant.SECURE,
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to add insecure level for this?

Copy link
Author

Choose a reason for hiding this comment

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

For the XSS in img tag ?

htmlTemplate = "LEVEL_1/XSS")
public ResponseEntity<String> getVulnerablePayloadLevelSecure4(@RequestParam(PARAMETER_NAME) String imageLocation) {
String hashedValue = hashSHA256(imageLocation);
if (hashedValue.equals("bd473875115034776f0fed141a0b6f8cbd46989e0ff1d52864f88a4e48882c75") ||
Copy link
Member

Choose a reason for hiding this comment

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

is this hashed values of zap image or owasp image? if so can you compute it on runtime. Why because it will help if we update the images, we don't need to change code.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we can do that but the ressources requested needs to be defined before running the function.

hashedValue.equals("6374f10c07ebcebe4dcff4df7ea882ab4e5feeb9ba132c94cd38ac880eb1407b")) {
String vulnerablePayloadWithPlaceHolder = "<img src=\"%s\" width=\"400\" height=\"300\"/>";
String payload = String.format(vulnerablePayloadWithPlaceHolder, imageLocation);
return new ResponseEntity<>(payload, HttpStatus.OK);
}
return new ResponseEntity<>("Invalid Image Hash", HttpStatus.BAD_REQUEST);
}

private String hashSHA256(String input) {
Copy link
Member

Choose a reason for hiding this comment

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

Move it to utils file so that we can utilise it at other places.

Copy link
Author

Choose a reason for hiding this comment

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

Noted !

try {
MessageDigest digest = MessageDigest.getInstance("SHA-256");
byte[] hash = digest.digest(input.getBytes());
return bytesToHex(hash);
} catch (NoSuchAlgorithmException e) {
return "";
}
}

private String bytesToHex(byte[] bytes) {
Copy link
Member

Choose a reason for hiding this comment

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

isn't there any utility already whcih does this for us?

StringBuilder hexString = new StringBuilder();
for (byte b : bytes) {
String hex = Integer.toHexString(0xff & b);
if (hex.length() == 1) {
hexString.append('0');
}
hexString.append(hex);
}
return hexString.toString();
}
}
3 changes: 3 additions & 0 deletions src/main/resources/i18n/messages_en_US.properties
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ XSS_HTML_ESCAPE_ON_DIRECT_INPUT_AND_REMOVAL_OF_VALUES_WITH_PARENTHESIS_SRC_ATTRI
XSS_QUOTES_AND_WITH_HTML_ESCAPE_ON_INPUT_SRC_ATTRIBUTE_IMG_TAG=HTML escaping is done on the Url Parameters and then inserted inside Quotes into the src attribute of Image Tag.
XSS_HTML_ESCAPE_PLUS_FILTERING_ON_INPUT_SRC_ATTRIBUTE_IMG_TAG_BUT_NULL_BYTE_VULNERABLE=Url Parameters are HTML escaped, validated against whitelist of filenames and inserted into the src attribute of Image Tag, However validator for validating filenames is vulnerable with Null Byte Injection.
XSS_QUOTES_AND_WITH_HTML_ESCAPE_PLUS_FILTERING_ON_INPUT_SRC_ATTRIBUTE_IMG_TAG=Url Parameters are HTML escaped, validated against whitelist of filenames and inserted inside Quotes into the src attribute of Image Tag.
XSS_CSP_PROTECTION="CSP is Used to prevent XSS"
XSS_MIME_TYPE_VALIDATION="MIME Type is tested to ensure that the file is PNG"
XSS_HASH_VALIDATION="Hash of the path is used to ensure that only the requested images are displayed"

## Html Tag Injection
XSS_HTML_TAG_INJECTION=Html Tag based XSS attack.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#SampleVulnerability {
color: black;
text-align: center;
}

#fetchDetails {
background: blueviolet;
display: inline-block;
padding: 8px 8px;
margin: 10px;
border: 2px solid transparent;
border-radius: 3px;
transition: 0.2s opacity;
color: #FFF;
font-size: 12px;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<div id="SampleVulnerability">
<div>
<div id="level_info">
This is a Sample Vulnerability. please add the UI components here.
</div>
<button id=fetchDetails>Click Here</button>
<div id="response"></div>
</div>
</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
function addingEventListenerToFetchData() {
Copy link
Member

Choose a reason for hiding this comment

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

please remove all sampleVulnerabilitu related files.

Copy link
Author

Choose a reason for hiding this comment

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

Yes i'll look at it.

document
.getElementById("fetchDetails")
.addEventListener("click", function () {
/**
* getUrlForVulnerabilityLevel() method provides url to call the Vulnerability Level
* of Sample Vulnerability.
* e.g. /VulnerableApp/SampleVulnerability/LEVEL_1 for LEVEL_1
*/
let url = getUrlForVulnerabilityLevel();
/**
* doGetAjaxCall() method is used to do the ajax get call to the Vulnerability Level
*/
doGetAjaxCall(fetchDataCallback, url + "?name=dummyInput", true);
});
}
// Used to register event on the button or any other component
addingEventListenerToFetchData();

//Callback function to handle the response and render in the UI
function fetchDataCallback(data) {
document.getElementById("response").innerHTML = data.content;
}
Loading