Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
Original file line number Diff line number Diff line change
Expand Up @@ -684,11 +684,7 @@ private void setSignatureValueElement(byte[] bytes) {
signatureValueElement.removeChild(signatureValueElement.getFirstChild());
}

String base64codedValue = XMLUtils.encodeToString(bytes);

if (base64codedValue.length() > 76 && !XMLUtils.ignoreLineBreaks()) {
base64codedValue = "\n" + base64codedValue + "\n";
}
String base64codedValue = XMLUtils.encodeElementValue(bytes);

Text t = createText(base64codedValue);
signatureValueElement.appendChild(t);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import javax.xml.stream.XMLStreamConstants;
import javax.xml.stream.XMLStreamException;

import org.apache.commons.codec.binary.Base64OutputStream;
import org.apache.xml.security.algorithms.JCEMapper;
import org.apache.xml.security.encryption.XMLCipherUtil;
import org.apache.xml.security.exceptions.XMLSecurityException;
Expand Down Expand Up @@ -175,12 +174,7 @@ public void init(OutputProcessorChain outputProcessorChain) throws XMLSecurityEx
symmetricCipher.init(Cipher.ENCRYPT_MODE, encryptionPartDef.getSymmetricKey(), parameterSpec);

characterEventGeneratorOutputStream = new CharacterEventGeneratorOutputStream();
Base64OutputStream base64EncoderStream = null; //NOPMD
if (XMLUtils.isIgnoreLineBreaks()) {
base64EncoderStream = new Base64OutputStream(characterEventGeneratorOutputStream, true, 0, null);
} else {
base64EncoderStream = new Base64OutputStream(characterEventGeneratorOutputStream, true);
}
OutputStream base64EncoderStream = XMLUtils.encodeStream(characterEventGeneratorOutputStream); //NOPMD
base64EncoderStream.write(iv);

OutputStream outputStream = new CipherOutputStream(base64EncoderStream, symmetricCipher); //NOPMD
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,9 +313,7 @@ public void addTextElement(String text, String localname) {
*/
public void addBase64Text(byte[] bytes) {
if (bytes != null) {
Text t = XMLUtils.ignoreLineBreaks()
? createText(XMLUtils.encodeToString(bytes))
: createText("\n" + XMLUtils.encodeToString(bytes) + "\n");
Text t = createText(XMLUtils.encodeElementValue(bytes));
appendSelf(t);
}
}
Expand Down
154 changes: 147 additions & 7 deletions src/main/java/org/apache/xml/security/utils/XMLUtils.java
Copy link
Member

Choose a reason for hiding this comment

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

Did you consider testing the properties by generating signed XML and then checking the format? This test won't catch issues where the internal methods are not called accidentally.

Copy link
Author

Choose a reason for hiding this comment

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

The fact that XMLUtils is a utility class with static methods, and the configuration can only be done with system properties at the class load time makes it hard to test. That's why the test contains custom class loader which reloads the class in each test, allowing to run static fields initialization in the middle of the test.

To build a sort of integration test against public API e.g. XMLSignature, I believe we will need to load XMLSignature and all the classes it depends on with this custom classloader. There are additional considerations regarding Jigsaw module system: since the new classloader loads classes in its own module, all packages from xmlsec which are not exported cannot be used by this classloader. I can try to implement such a test, but I'm afraid it will be totally unreadable...

Intuitively, a good test interacts with the code in a way it is intended to be used. In our case, parts of the public interface are system properties. It looks proper to run the test suite with different JVMs started with different properties. Probably, Maven has something to achieve this, I'll investigate.

On the other hand, we can make it more testable (and probably a little more convenient for usage) if we provide a way to set Base64 options in runtime, for example:

  • refer to system properties and resolve the options each time we do base64 encoding (e.g. inside encodeToString etc.)
  • add public setters for base64FormattingOptions and ignoreLineBreaks
  • add public setter for base64Encoder - then we don't need new options at all. We can default to Base64.getEncoder() / Base64.getMimeEncoder() depending on ignoreLineBreaks, and if the clients need custom formatting - they just provide any implementation of Base64.Encoder

However, I'm not aware of security implications behind this...

All other inline comments were addressed in 8a598d0. Thank you for checking the code carefully.

Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,38 @@
/**
* DOM and XML accessibility and comfort functions.
*
* @implNote
* The following system properties affect XML formatting:
* <ul>
* <li>{@systemProperty org.apache.xml.security.ignoreLineBreaks} - ignores all line breaks,
* making a single-line document. Overrides all other formatting options. Default: false</li>
* <li>{@systemProperty org.apache.xml.security.base64.ignoreLineBreaks} - ignores line breaks in base64Binary values.
* Takes precedence over line length and separator options (see below). Default: false</li>
* <li>{@systemProperty org.apache.xml.security.base64.lineSeparator} - Sets the line separator sequence in base64Binary values.
* Possible values: crlf, lf. Default: crlf</li>
* <li>{@systemProperty org.apache.xml.security.base64.lineLength} - Sets maximum line length in base64Binary values.
* The value is rounded down to the nearest multiple of 4. Values less than 4 are ignored. Default: 76</li>
* </ul>
*/
public final class XMLUtils {

private static final Logger LOG = System.getLogger(XMLUtils.class.getName());

private static final String IGNORE_LINE_BREAKS_PROP = "org.apache.xml.security.ignoreLineBreaks";

private static boolean ignoreLineBreaks =
AccessController.doPrivileged(
(PrivilegedAction<Boolean>) () -> Boolean.getBoolean("org.apache.xml.security.ignoreLineBreaks"));
(PrivilegedAction<Boolean>) () -> Boolean.getBoolean(IGNORE_LINE_BREAKS_PROP));

private static final Logger LOG = System.getLogger(XMLUtils.class.getName());
private static Base64FormattingOptions base64Formatting =
Copy link
Member

Choose a reason for hiding this comment

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

if org.apache.xml.security.ignoreLineBreaks is true, none of these options matter, so did you consider skipping the getting of the other properties?

Copy link
Member

Choose a reason for hiding this comment

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

Also should log a warning if the other properties are set, since they have no impact if org.apache.xml.security.ignoreLineBreaks is true.

AccessController.doPrivileged(
(PrivilegedAction<Base64FormattingOptions>) () -> new Base64FormattingOptions());

private static Base64.Encoder base64Encoder = (ignoreLineBreaks || base64Formatting.isIgnoreLineBreaks()) ?
Base64.getEncoder() :
Base64.getMimeEncoder(base64Formatting.getLineLength(), base64Formatting.getLineSeparator().getBytes());

private static Base64.Decoder base64Decoder = Base64.getMimeDecoder();

private static XMLParser xmlParserImpl =
AccessController.doPrivileged(
Expand Down Expand Up @@ -515,18 +539,48 @@ public static void addReturnBeforeChild(Element e, Node child) {
}

public static String encodeToString(byte[] bytes) {
if (ignoreLineBreaks) {
return Base64.getEncoder().encodeToString(bytes);
return base64Encoder.encodeToString(bytes);
}

/**
* Encodes bytes using Base64, with or without line breaks, depending on configuration (see {@link XMLUtils}).
* @param bytes Bytes to encode
* @return Base64 string
*/
public static String encodeElementValue(byte[] bytes) {
String encoded = encodeToString(bytes);
if (!ignoreLineBreaks && !base64Formatting.isIgnoreLineBreaks()
&& encoded.length() > base64Formatting.getLineLength()) {
encoded = "\n" + encoded + "\n";
}
return Base64.getMimeEncoder().encodeToString(bytes);
return encoded;
}

/**
* Wraps output stream for Base64 encoding.
* Output data may contain line breaks or not, depending on configuration (see {@link XMLUtils})
* @param stream The underlying output stream to write Base64-encoded data
* @return Stream which writes binary data using Base64 encoder
*/
public static OutputStream encodeStream(OutputStream stream) {
return base64Encoder.wrap(stream);
}

public static byte[] decode(String encodedString) {
return Base64.getMimeDecoder().decode(encodedString);
return base64Decoder.decode(encodedString);
}

public static byte[] decode(byte[] encodedBytes) {
return Base64.getMimeDecoder().decode(encodedBytes);
return base64Decoder.decode(encodedBytes);
}

/**
* Wraps input stream for Base64 decoding.
* @param stream Input stream with Base64-encoded data
* @return Input stream with decoded binary data
*/
public static InputStream decodeStream(InputStream stream) {
return base64Decoder.wrap(stream);
}

public static boolean isIgnoreLineBreaks() {
Expand Down Expand Up @@ -1068,4 +1122,90 @@ public static byte[] getBytes(BigInteger big, int bitlen) {

return resizedBytes;
}

/**
* Aggregates formatting options for base64Binary values.
*/
static class Base64FormattingOptions {
private static final String BASE64_IGNORE_LINE_BREAKS_PROP = "org.apache.xml.security.base64.ignoreLineBreaks";
private static final String BASE64_LINE_SEPARATOR_PROP = "org.apache.xml.security.base64.lineSeparator";
private static final String BASE64_LINE_LENGTH_PROP = "org.apache.xml.security.base64.lineLength";

private boolean ignoreLineBreaks = false;
private Base64LineSeparator lineSeparator = Base64LineSeparator.CRLF;
private int lineLength = 76;

/**
* Creates new formatting options by reading system properties.
*/
public Base64FormattingOptions() {
Copy link
Member

Choose a reason for hiding this comment

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

The constructors and methods of this class should be package-private since the class is package-private.

String ignoreLineBreaksProp = System.getProperty(BASE64_IGNORE_LINE_BREAKS_PROP);
ignoreLineBreaks = Boolean.parseBoolean(ignoreLineBreaksProp);
if (XMLUtils.ignoreLineBreaks && ignoreLineBreaksProp != null && !ignoreLineBreaks) {
LOG.log(Level.WARNING, "{0} property takes precedence over {1}, line breaks will be ignored",
IGNORE_LINE_BREAKS_PROP, BASE64_IGNORE_LINE_BREAKS_PROP);
}

String lineSeparatorProp = System.getProperty(BASE64_LINE_SEPARATOR_PROP);
if (lineSeparatorProp != null) {
try {
lineSeparator = Base64LineSeparator.valueOf(lineSeparatorProp.toUpperCase());
if (XMLUtils.ignoreLineBreaks || ignoreLineBreaks) {
LOG.log(Level.WARNING, "Property {0} has no effect since line breaks are ignored",
BASE64_LINE_SEPARATOR_PROP);
}
} catch (IllegalArgumentException e) {
LOG.log(Level.WARNING, "Illegal value of {0} property is ignored: {1}",
BASE64_LINE_SEPARATOR_PROP, lineSeparatorProp);
}
}

String lineLengthProp = System.getProperty(BASE64_LINE_LENGTH_PROP);
if (lineLengthProp != null) {
try {
int lineLength = Integer.parseInt(lineLengthProp);
if (lineLength >= 4) {
this.lineLength = lineLength;
if (XMLUtils.ignoreLineBreaks || ignoreLineBreaks) {
LOG.log(Level.WARNING, "Property {0} has no effect since line breaks are ignored",
BASE64_LINE_LENGTH_PROP);
}
} else {
LOG.log(Level.WARNING, "Illegal value of {0} property is ignored: {1}",
BASE64_LINE_LENGTH_PROP, lineLengthProp);
}
} catch (NumberFormatException e) {
LOG.log(Level.WARNING, "Illegal value of {0} property is ignored: {1}",
BASE64_LINE_LENGTH_PROP, lineLengthProp);
}
}
}

public boolean isIgnoreLineBreaks() {
return ignoreLineBreaks;
}

public Base64LineSeparator getLineSeparator() {
return lineSeparator;
}

public int getLineLength() {
return lineLength;
}
}

enum Base64LineSeparator {
CRLF(new byte[]{'\r', '\n'}),
LF(new byte[]{'\n'});

private byte[] bytes;

Base64LineSeparator(byte[] bytes) {
this.bytes = bytes;
}

public byte[] getBytes() {
Copy link
Member

Choose a reason for hiding this comment

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

Change to package-private.

return bytes;
}
}
}
Loading