You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@santuario.apache.org by GitBox <gi...@apache.org> on 2021/09/02 08:20:46 UTC
[GitHub] [santuario-xml-security-java] coheigea opened a new pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
coheigea opened a new pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58
…eferences are allowed from an unsigned context
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] seanjmullan commented on a change in pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
seanjmullan commented on a change in pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58#discussion_r701231380
##########
File path: src/main/java/org/apache/xml/security/utils/resolver/ResourceResolverContext.java
##########
@@ -51,4 +57,12 @@ public ResourceResolverContext(Attr attr, String baseUri, boolean secureValidati
return properties;
}
+ public boolean isSafeURIToResolve() {
+ if (allowUnsafeResourceResolving) {
+ return true;
+ }
+ boolean forbiddenURI = (uriToResolve != null && (uriToResolve.startsWith("file:") || uriToResolve.startsWith("http:")))
Review comment:
Unless this is done somewhere else, I think you should lowercase `uriToResolve` (probably in the ctor) so that it also matches mixed/upper case schemes, ex "HTTP:".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea commented on a change in pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
coheigea commented on a change in pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58#discussion_r702684294
##########
File path: src/main/java/org/apache/xml/security/utils/resolver/ResourceResolverContext.java
##########
@@ -51,4 +57,12 @@ public ResourceResolverContext(Attr attr, String baseUri, boolean secureValidati
return properties;
}
+ public boolean isSafeURIToResolve() {
+ if (allowUnsafeResourceResolving) {
+ return true;
+ }
+ boolean forbiddenURI = (uriToResolve != null && (uriToResolve.startsWith("file:") || uriToResolve.startsWith("http:")))
Review comment:
I think we don't need to do this, as the resource resolvers only handle lower case file/http URLs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea merged pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
coheigea merged pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] seanjmullan commented on a change in pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
seanjmullan commented on a change in pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58#discussion_r703504433
##########
File path: src/test/java/org/apache/xml/security/test/dom/utils/resolver/ResourceResolverTest.java
##########
@@ -96,4 +98,65 @@ public void testLocalFileWithEmptyBaseURI() throws Exception {
}
}
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveFile() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ String file = new File(basedir, "pom.xml").toURI().toString();
+ uriAttr.setValue(file);
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, null, false);
+ assertFalse(resolverContext.isURISafeToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveFileBaseURI() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ String file = new File(basedir, "pom.xml").toURI().toString();
+ uriAttr.setValue("xyz");
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, file, false);
+ assertFalse(resolverContext.isURISafeToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveHTTP() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
Review comment:
unused variable
##########
File path: src/main/java/org/apache/xml/security/encryption/XMLCipherInput.java
##########
@@ -115,7 +115,14 @@ public void setSecureValidation(boolean secureValidation) {
try {
ResourceResolverContext resolverContext =
new ResourceResolverContext(uriAttr, null, secureValidation);
- input = ResourceResolver.resolve(resolverContext);
+ if (resolverContext.isURISafeToResolve()) {
+ input = ResourceResolver.resolve(resolverContext);
+ } else {
+ String uriToResolve = uriAttr != null ? uriAttr.getValue() : null;
+ Object[] exArgs = {uriToResolve != null ? uriToResolve : "null", null};
Review comment:
I don't think you need to set the first argument to `"null"` if `uriToResolve` is `null`. I think the exception message should be able to handle `null` arguments, i.e. `MessageFormat.format` should print `"null"` for `null` arguments.
##########
File path: src/test/java/org/apache/xml/security/test/dom/utils/resolver/ResourceResolverTest.java
##########
@@ -96,4 +98,65 @@ public void testLocalFileWithEmptyBaseURI() throws Exception {
}
}
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveFile() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ String file = new File(basedir, "pom.xml").toURI().toString();
+ uriAttr.setValue(file);
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, null, false);
+ assertFalse(resolverContext.isURISafeToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveFileBaseURI() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ String file = new File(basedir, "pom.xml").toURI().toString();
+ uriAttr.setValue("xyz");
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, file, false);
+ assertFalse(resolverContext.isURISafeToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveHTTP() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ uriAttr.setValue("http://www.apache.org");
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, null, false);
+ assertFalse(resolverContext.isURISafeToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveHTTPBaseURI() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
Review comment:
unused variable
##########
File path: src/test/java/org/apache/xml/security/test/dom/utils/resolver/ResourceResolverTest.java
##########
@@ -96,4 +98,65 @@ public void testLocalFileWithEmptyBaseURI() throws Exception {
}
}
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveFile() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ String file = new File(basedir, "pom.xml").toURI().toString();
+ uriAttr.setValue(file);
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, null, false);
+ assertFalse(resolverContext.isURISafeToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveFileBaseURI() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ String file = new File(basedir, "pom.xml").toURI().toString();
+ uriAttr.setValue("xyz");
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, file, false);
+ assertFalse(resolverContext.isURISafeToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveHTTP() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ uriAttr.setValue("http://www.apache.org");
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, null, false);
+ assertFalse(resolverContext.isURISafeToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveHTTPBaseURI() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ uriAttr.setValue("xyz");
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, "http://www.apache.org", false);
+ assertFalse(resolverContext.isURISafeToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveLocalReference() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
Review comment:
unused variable
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] seanjmullan commented on a change in pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
seanjmullan commented on a change in pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58#discussion_r701302753
##########
File path: src/test/java/org/apache/xml/security/test/dom/utils/resolver/ResourceResolverTest.java
##########
@@ -96,4 +98,40 @@ public void testLocalFileWithEmptyBaseURI() throws Exception {
}
}
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveFile() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ String file = new File(basedir, "pom.xml").toURI().toString();
+ uriAttr.setValue(file);
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, null, false);
+ assertFalse(resolverContext.isSafeURIToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveHTTP() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ uriAttr.setValue("http://www.apache.org");
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, null, false);
+ assertFalse(resolverContext.isSafeURIToResolve());
+ }
+
+ @org.junit.jupiter.api.Test
+ public void testIsSafeURIToResolveLocalReference() throws Exception {
+ Document doc = TestUtils.newDocument();
+ Attr uriAttr = doc.createAttribute("URI");
+ String basedir = System.getProperty("basedir");
+ uriAttr.setValue("#1234");
+
+ ResourceResolverContext resolverContext =
+ new ResourceResolverContext(uriAttr, null, false);
+ assertTrue(resolverContext.isSafeURIToResolve());
+ }
Review comment:
How about also adding a test with a file or http baseUri?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] seanjmullan commented on a change in pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
seanjmullan commented on a change in pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58#discussion_r701299949
##########
File path: src/main/java/org/apache/xml/security/utils/resolver/ResourceResolverContext.java
##########
@@ -51,4 +57,12 @@ public ResourceResolverContext(Attr attr, String baseUri, boolean secureValidati
return properties;
}
+ public boolean isSafeURIToResolve() {
Review comment:
`isURISafeToResolve` sounds better to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] seanjmullan commented on a change in pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
seanjmullan commented on a change in pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58#discussion_r701286783
##########
File path: src/main/java/org/apache/xml/security/utils/resolver/ResourceResolverContext.java
##########
@@ -51,4 +57,12 @@ public ResourceResolverContext(Attr attr, String baseUri, boolean secureValidati
return properties;
}
+ public boolean isSafeURIToResolve() {
+ if (allowUnsafeResourceResolving) {
+ return true;
+ }
+ boolean forbiddenURI = (uriToResolve != null && (uriToResolve.startsWith("file:") || uriToResolve.startsWith("http:")))
+ || (baseUri != null && (baseUri.startsWith("file:") || baseUri.startsWith("http:")));
Review comment:
For the baseUri, you can make this check more precise by only checking relative URIs. This way you won't trigger false cases where a baseURI has been set for resolving some other URI but not an unsafe reference URI. Something like:
`if (baseUri != null && !baseUri.isEmpty() && baseUri.charAt(0) != '#') {
// now check if it is file or http ...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] seanjmullan commented on a change in pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
seanjmullan commented on a change in pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58#discussion_r701286783
##########
File path: src/main/java/org/apache/xml/security/utils/resolver/ResourceResolverContext.java
##########
@@ -51,4 +57,12 @@ public ResourceResolverContext(Attr attr, String baseUri, boolean secureValidati
return properties;
}
+ public boolean isSafeURIToResolve() {
+ if (allowUnsafeResourceResolving) {
+ return true;
+ }
+ boolean forbiddenURI = (uriToResolve != null && (uriToResolve.startsWith("file:") || uriToResolve.startsWith("http:")))
+ || (baseUri != null && (baseUri.startsWith("file:") || baseUri.startsWith("http:")));
Review comment:
For the baseUri, you can make this check more precise by only checking relative URIs. This way you won't trigger false cases where a baseURI has been set for resolving some other URI but not an unsafe reference URI. Something like:
`if (uriToResolve != null && !uriToResolve.isEmpty() && uriToResolve.charAt(0) != '#') {
// now check if baseUri is file or http ...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] seanjmullan commented on a change in pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
seanjmullan commented on a change in pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58#discussion_r703475804
##########
File path: src/main/java/org/apache/jcp/xml/dsig/internal/dom/DOMURIDereferencer.java
##########
@@ -102,16 +102,19 @@ public Data dereference(URIReference uriRef, XMLCryptoContext context)
}
}
- try {
- ResourceResolverContext resContext = new ResourceResolverContext(uriAttr, baseURI, secVal);
- XMLSignatureInput in = ResourceResolver.resolve(resContext);
- if (in.isOctetStream()) {
- return new ApacheOctetStreamData(in);
- } else {
- return new ApacheNodeSetData(in);
+ ResourceResolverContext resContext = new ResourceResolverContext(uriAttr, baseURI, secVal);
+ if ((uriRef instanceof javax.xml.crypto.dsig.Reference) || resContext.isURISafeToResolve()) {
+ try {
+ XMLSignatureInput in = ResourceResolver.resolve(resContext);
+ if (in.isOctetStream()) {
+ return new ApacheOctetStreamData(in);
+ } else {
+ return new ApacheNodeSetData(in);
+ }
+ } catch (Exception e) {
+ throw new URIReferenceException(e);
}
- } catch (Exception e) {
- throw new URIReferenceException(e);
}
+ throw new URIReferenceException("Uri " + uri + " is forbidden");
Review comment:
Just a nit, change "Uri" to "URI" to be consistent with attribute name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] coheigea commented on a change in pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
coheigea commented on a change in pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58#discussion_r703487183
##########
File path: src/main/java/org/apache/jcp/xml/dsig/internal/dom/DOMURIDereferencer.java
##########
@@ -102,16 +102,19 @@ public Data dereference(URIReference uriRef, XMLCryptoContext context)
}
}
- try {
- ResourceResolverContext resContext = new ResourceResolverContext(uriAttr, baseURI, secVal);
- XMLSignatureInput in = ResourceResolver.resolve(resContext);
- if (in.isOctetStream()) {
- return new ApacheOctetStreamData(in);
- } else {
- return new ApacheNodeSetData(in);
+ ResourceResolverContext resContext = new ResourceResolverContext(uriAttr, baseURI, secVal);
+ if ((uriRef instanceof javax.xml.crypto.dsig.Reference) || resContext.isURISafeToResolve()) {
+ try {
+ XMLSignatureInput in = ResourceResolver.resolve(resContext);
+ if (in.isOctetStream()) {
+ return new ApacheOctetStreamData(in);
+ } else {
+ return new ApacheNodeSetData(in);
+ }
+ } catch (Exception e) {
+ throw new URIReferenceException(e);
}
- } catch (Exception e) {
- throw new URIReferenceException(e);
}
+ throw new URIReferenceException("Uri " + uri + " is forbidden");
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [santuario-xml-security-java] seanjmullan commented on a change in pull request #58: SANTUARIO-577 - Introduce a system property to control if file/http r…
Posted by GitBox <gi...@apache.org>.
seanjmullan commented on a change in pull request #58:
URL: https://github.com/apache/santuario-xml-security-java/pull/58#discussion_r701299949
##########
File path: src/main/java/org/apache/xml/security/utils/resolver/ResourceResolverContext.java
##########
@@ -51,4 +57,12 @@ public ResourceResolverContext(Attr attr, String baseUri, boolean secureValidati
return properties;
}
+ public boolean isSafeURIToResolve() {
Review comment:
`isURISafeToResolve` sounds more better to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@santuario.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org