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.

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

SRI support #235

Merged
merged 6 commits into from
Jun 30, 2017
Merged

SRI support #235

merged 6 commits into from
Jun 30, 2017

Conversation

SanBlig
Copy link
Contributor

@SanBlig SanBlig commented Jun 22, 2017 •
edited by hrj
Loading

Closes #234

This provides partial support for Subresource Integrity.

The following are yet to be considered:

  1. Cache:
    • A response served through cache must undergo integrity check.
    • If integrity check for a response fails, it shouldn't be added to the cache.
  2. Check eligibility of response for integrity validation - CORS.
  3. Check the syntax before parsing integrity tokens (probably redundant).
  4. Support for options as part of the integrity metadata.

@hrj
Copy link
Member

hrj commented Jun 22, 2017

Excellent !

Adding to the list of things to do:

  • enumerate all the types of requests that can be checked for integrity. Top of my mind: requests from img and iframe elements
  • enumerate the protocols that can be checked for integrity. http:// and https:// would be already covered. Check data as well.

hrj
hrj requested changes Jun 22, 2017
View reviewed changes
src/HTML_Renderer/org/lobobrowser/html/domimpl/HTMLLinkElementImpl.java Outdated
@@ -128,6 +128,10 @@ public void setType(final String type) {
this.setAttribute("type", type);
}

public String getIntegrity() {
return this.getAttribute("integrity");
Copy link
Member

Choose a reason for hiding this comment

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

minor: check indent

Copy link
Member

Choose a reason for hiding this comment

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

Also, make the function private, unless public is necessary.

src/HTML_Renderer/org/lobobrowser/html/domimpl/HTMLScriptElementImpl.java Outdated
@@ -108,6 +108,10 @@ public void setType(final String type) {
this.setAttribute("type", type);
}

public String getIntegrity() {
return this.getAttribute("integrity");
}
Copy link
Member

Choose a reason for hiding this comment

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

minor: check indent

also check visibility (private / public)

src/HTML_Renderer/org/lobobrowser/html/style/CSSUtilities.java Outdated
logger.warning("Unable to parse CSS. URI=[" + cssURI + "]. Response status was " + status + ".");
return getEmptyStyleSheet();
}
*/
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 the commented code

src/Platform_Core/org/lobobrowser/context/AlgorithmDigest.java Outdated
import java.util.Map;
import java.util.stream.Collectors;

public class AlgorithmDigest implements Comparable<AlgorithmDigest> {
Copy link
Member

Choose a reason for hiding this comment

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

check visibility of class: package private / public

Also make it final.

src/Platform_Core/org/lobobrowser/context/AlgorithmDigest.java Outdated
public static boolean validate(final byte[] input, final String integrity) {

final List<AlgorithmDigest> algDigests = parseMetadata(integrity);
System.out.println("Strongest algorithms: " + algDigests);
Copy link
Member

Choose a reason for hiding this comment

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

nuke all debug prints in this file

src/Platform_Core/org/lobobrowser/context/AlgorithmDigest.java Outdated
System.out.println("The value of the integrity attribute is: " + integrity);

if (algDigests == null)
return true;
Copy link
Member

Choose a reason for hiding this comment

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

code style: surround with braces.

For a classic example of the problem that braces protect against, see: https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/

src/Platform_Core/org/lobobrowser/context/AlgorithmDigest.java Outdated
private static List<AlgorithmDigest> getHashes(final String[] tokens) {
final List<AlgorithmDigest> hashes = new ArrayList<AlgorithmDigest>();

for (int i = 0; i < tokens.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Try converting to enhanced for loop. In eclipse Ctrl 1 should suggest it automatically.

src/Platform_Core/org/lobobrowser/context/AlgorithmDigest.java Outdated
} catch (final Exception e) {
System.out.println("Exception: " + e.getMessage());
}
return "";
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to return null on failure, instead of empty string.

src/Platform_Core/org/lobobrowser/context/NetworkRequestImpl.java Outdated
@@ -251,7 +256,7 @@ private void setResponse(final ClientletResponse response) {
}
try {
this.changeReadyState(NetworkRequest.STATE_LOADING);
final LocalResponse newResponse = new LocalResponse(response);
LocalResponse newResponse = new LocalResponse(response); //was final before
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it can be converted back to final.

SanBlig added 4 commits June 29, 2017 23:25
@SanBlig
Made a few formatting changes, redefined scope in some places and rem…
5702c60 
…oved all debug prints.
@SanBlig
Merge remote-tracking branch 'origin/master' into sri
b3110b6
@SanBlig
Support for Cache
eb16d74 
The following actions take place:
  - If response is from cache, it undergoes integrity check.
  - If response fails integrity check, it is not added to cache.
@SanBlig
Implementation changes for cache support
3d9e4e9 
Used the predefined functional interface Consumer<T> instead of Valid. The latter has been deleted. setResponse now takes Consumer<Boolean> as a parameter and calls accept() instead of returning a boolean value.
hrj
hrj requested changes Jun 30, 2017
View reviewed changes
Copy link
Member

@hrj hrj left a comment

Choose a reason for hiding this comment

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

This PR has come far. Kudos!

Just a few changes remaining.

src/Platform_Core/org/lobobrowser/context/NetworkRequestImpl.java Outdated
final Runnable runnable = () -> {
if (response.isFromCache()) {
final Object cachedResponse = response.getTransientCachedObject();
if (cachedResponse instanceof CacheableResponse) {
System.out.println("Cached response for " + response.getResponseURL());
Copy link
Member

Choose a reason for hiding this comment

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

remove debug print

src/Platform_Core/org/lobobrowser/context/NetworkRequestImpl.java Outdated
}

/*
* (non-Javadoc)
*
* @see net.sourceforge.xamj.http.RequestHandler#handleProgress(int,
* java.net.URL, int, int)
*void java.net.URL, int, int)
Copy link
Member

Choose a reason for hiding this comment

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

this looks like an accidental change

src/Platform_Core/org/lobobrowser/request/ClientletRequestHandler.java
@@ -97,7 +98,7 @@ public boolean handleException(final ClientletResponse response, final Throwable
* .ClientletResponse)
*/
@Override
public void processResponse(final ClientletResponse response) throws ClientletException, IOException {
public void processResponse(final ClientletResponse response, Consumer<Boolean> consumer) throws ClientletException, IOException {
Copy link
Member

Choose a reason for hiding this comment

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

In all these other processResponse implementations, the response is "valid", so you will have to call consumer.accept(true) somewhere in the function (most likely at the end of the function).

src/Platform_Public_API/org/lobobrowser/ua/NetworkRequest.java Outdated
* @param userName
* The HTTP authentication user name.
* @param integrity
* Integrity value to be checked.
Copy link
Member

Choose a reason for hiding this comment

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

This also looks like a spurious change.

@SanBlig
Minor changes
8b4748c 
Accidental and spurious changes undone. Added calls to consumer.accept(true) where absent.
@hrj hrj changed the title [WIP] SRI support SRI support Jun 30, 2017
@hrj hrj merged commit 044f400 into gngrOrg:master Jun 30, 2017
@hrj
Copy link
Member

hrj commented Jun 30, 2017

Thanks @SanBlig

I added support for options in f74e263

Other pending stuff has been copied into a new issue: #237

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

@hrj hrj hrj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SanBlig @hrj