coheigea commented on code in PR #3504:
URL: https://github.com/apache/cxf/pull/3504#discussion_r4105373492
##########
rt/rs/security/xml/src/main/java/org/apache/cxf/rs/security/saml/SamlEnvelopedInHandler.java:
##########
@@ -85,7 +93,11 @@ public void filter(ContainerRequestContext context) {
validateToken(message, samlElement);
doc.getDocumentElement().removeChild(samlElement);
- if (bodyIsRoot) {
+ if (signedBody != null) {
+ message.setContent(XMLStreamReader.class,
Review Comment:
Right, the owning document is kept, but only for
getDocument()/getSystemId(). The events the reader emits start at the node it
was constructed with. So new W3CDOMStreamReader(signedBody) streams only the
signed element, while new W3CDOMStreamReader(doc) streams the whole document,
including any unsigned content in the wrapper. That's why we need the
distinction. With a detached signature, AbstractXmlSigInHandler narrows the
reader to the signed element, and this handler still uses getDocument() to find
the SAML assertion (a sibling outside the signed element). Then it has to keep
passing on only the signed element, rather than rebuilding the reader from the
root or taking its first child, which an attacker can control.
testDetachedSignatureWithUnsignedContent covers the narrowing.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
