JDK-7160837 : DigestOutputStream does not turn off digest calculation when "close()" is called
  • Type: Bug
  • Component: security-libs
  • Sub-Component: javax.crypto
  • Affected Version: 7
  • Priority: P3
  • Status: Closed
  • Resolution: Fixed
  • OS: windows_7
  • CPU: x86
  • Submitted: 2012-04-12
  • Updated: 2015-06-30
  • Resolved: 2013-05-31
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other Other JDK 6 JDK 7 JDK 8
5.0u75Fixed 5.0u81Fixed 6u105Fixed 7u71Fixed 8 b94Fixed
Related Reports
Relates :  
Description
FULL PRODUCT VERSION :
java version "1.7.0_03"
Java(TM) SE Runtime Environment (build 1.7.0_03-b05)
Java HotSpot(TM) Client VM (build 22.1-b02, mixed mode, sharing)

ADDITIONAL OS VERSION INFORMATION :
Microsoft Windows [Version 6.1.7601]

EXTRA RELEVANT SYSTEM CONFIGURATION :
CPU: Intel Core i7 2600K
RAM: 8GB

A DESCRIPTION OF THE PROBLEM :
As DigestOutputStream does not implement the FilterOutputStream.close() method, it has no awareness of the stream being actually closed. When the stream is closed and digest calculation is left "on", this object continues updating its MessageDigest on subsequent calls on any of its write() methods.

This causes problem in case of nested OutputStreams. In particular, nesting an ObjectOutputStream over a CipherOutputStream (with block ciphers such as AES) which is on top of a DigestOutputStream reveals the problem, that is, when we are finished with the inner most stream (the ObjectOutputSteam in this case), we have to call on its close method. This call, in turn, forces a flush on CipherOutputStream which DOES NOT FLUSH its unprocessed buffers. Then comes the turn for CipherOutputSteam being closed, which first performs a flush and then a final output of its residue buffers to the underlying stream (DigestOutputStream), followed by a flush and close on DigestOutputStream.

Things are pretty clear up to this point, but for some strange reasons, CipherOutputSteam decides to do a flush and re-write its latest buffer values after it has closed its underlying stream. Since DigestOutputStream has not taken last "close()" call seriously, it accepts the "write()" call and updates its digest AND THEN passes the call to the underlying stream  which reasonably throws a "Stream Closed" IOExcepion, but since the CipherOutputStram is already closed, it will not re-throw the exception and no one on the outside will eve be told of this. But, the one who is left in the dark is the poor DigestOutputStream which has already updated its digest WITHOUT first taking into account its state and to a lesser degree WITHOUT waiting for an underlying "write()" to succeed.

It might be more of a problem by CipherOutputStream calling "write()" after being closed, but more or less, it is also a flaw in DigestOutputStream.


STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
1. Create a FileOutputStream
2. Create a DigestOutputStream (SHA-1) over it
3. Initialize a block-cipher (AES-128) and create a CipherOutputStream over the previous one
4. Create an ObjectOutputStream
5. Write some data to the ObjectOutputStream
6. Close the ObjectOutputStream
7. Close the CipherOutputSteam (if necessary)
8. Close the DigestOutputStream (if necessary)
9. Close the FileOutputStream (if necessary)
10. Call "digest()" on the MessageDigest object of the DigestOutputStream and store the result.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
The result stored in step 10 must be equal to an independently calculated SHA-1 checksum of the file contents, which is surprisingly not, as mentioned earlier.
ACTUAL -
A different checksum, due to a late call to "write()" method which INCORRECTLY updated the message digest.

ERROR MESSAGES/STACK TRACES THAT OCCUR :
no error message is produces here

REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.ObjectOutputStream;
import java.security.DigestOutputStream;
import java.security.MessageDigest;
import java.util.Arrays;

import javax.crypto.Cipher;
import javax.crypto.CipherOutputStream;
import javax.crypto.KeyGenerator;
import javax.crypto.SecretKey;

public class Test
{
	public static void main(String[] args) throws Exception
	{
		/**
		 * Producing the problem
		 */
		MessageDigest sha1 = MessageDigest.getInstance("SHA1");
		KeyGenerator aesKeyGen = KeyGenerator.getInstance("AES");
		aesKeyGen.init(128);
		SecretKey aesKey = aesKeyGen.generateKey();
		Cipher aes = Cipher.getInstance("AES");
		aes.init(Cipher.ENCRYPT_MODE, aesKey);
		try (FileOutputStream fos = new FileOutputStream("test.bin");
				DigestOutputStream dos = new DigestOutputStream(fos, sha1);
				CipherOutputStream cos = new CipherOutputStream(dos, aes);
				ObjectOutputStream oos = new ObjectOutputStream(cos))
		{
			oos.writeObject("This is a sample data to be AES encrypted.");
		}
		byte[] incorrectSha1 = sha1.digest();

		/**
		 * Calculating the correct SHA1 for the file
		 */
		byte[] correctSha1 = null;
		sha1.reset();
		try (FileInputStream fis = new FileInputStream("test.bin"))
		{
			byte[] buffer = new byte[1024];
			int count;
			count = fis.read(buffer);

			sha1.update(buffer, 0, count);
			correctSha1 = sha1.digest();
		}

		if(Arrays.equals(correctSha1, incorrectSha1) == false)
			System.out.println("The flaw still exists!");
		else
			System.out.println("The problem is resolved.");
	}
}

---------- END SOURCE ----------

CUSTOMER SUBMITTED WORKAROUND :
The simplest solution to work this around is to subclass DigestOutputStream, override the "close()" method and TURN ITS CALCULATIONS OFF by calling "on(false)" after calling the "super.close()". Following snippet demonstrate it:

public static class CorrectDigestOutputStream extends DigestOutputStream
	{

		public CorrectDigestOutputStream(OutputStream stream, MessageDigest digest)
		{
			super(stream, digest);
		}

		@Override
		public void close() throws IOException
		{
			super.close();
			this.on(false);
		}
	}

We might be able to fix it by overriding the "write()" methods, but this one is more concise.

Comments
provided test javax/crypto/Cipher/CipherStreamClose.java have passed in B94 and B95 in 1.8
18-06-2013

I'm sticking with fixing the bug as reported. Some additional comments regarding the change don't seem as severe to me and others go beyond what. There is no evidence that this methods need synchronized and such a change would require performance testing.
14-05-2013

EVALUATION I think the following needs to be done: 1) In both CipherOutputStream and DigestOutputStream, if closed, additional write operations need to throw some kind of IOException. The API doesn't need to be changed, but a CCC will need to be filed since we will now be enforcing this behavior. 2) These methods should probably be syncronized. 3) You should check to see if the same needs to be done on the InputStreams as well. 4) Normally when an OutputStream.close() is called, the underlying resources are supposed to be released (see FilterOutputStream.close). It's not clear to me whether the underlying resources MessageDigest should be released when the stream is closed. For example, DigestOutputStream.getMessageDigest() is supposed to return the MessageDigest associated with that OutputStream. If we change it, the call sequence: dos.close(); md = dos.getMessageDigest(); // digest was released above. md.digest(); will fail but: md = dos.getMessageDigest(); dos.close(); md.digest(); I think we'd be ok if we released the Cipher in CipherOutputStream since there is no way to reobtain it, but I don't think we should do this on DigestOutputStream. To be considered more...
15-06-2012

EVALUATION Also, CipherOutputStream.close has contradictory information in the close section. * Closes this output stream and releases any system resources * associated with this stream. * <p> * This method invokes the <code>doFinal</code> method of the encapsulated * cipher object, which causes any bytes buffered by the encapsulated * cipher to be processed. The result is written out by calling the * <code>flush</code> method of this output stream. * <p> * This method resets the encapsulated cipher object to its initial state * and calls the <code>close</code> method of the underlying output * stream. This needs to be clarified that: 1) When close is called, like other InputStreams, the underlying resources are released. (see Evaluation #3 below for whether this actually should be done) 2) underlying cipher is reset. 3) close on the underlying output stream is done.
14-04-2012

EVALUATION The fact that this hasn't been seen yet indicates not a lot of use in this particular combination. CipherOutputStream is doing a double close due to the try-with-resources block: first from the ObjectOutputStream, and once from the CipherOutputStream, which in turn in sending off an extra 16 byte packet (consisting of padding). Fortunately, that packet is getting caught by the lower levels and is not actually written to the FileOutputStream. This extra 16 byte packet definitely needs to be fixed. Restating: What's happening is a stream is being created: ObjectOutputStream -> CipherOutputStream -> DigestOutputStream -> FileOutputStream The goal is to write encrypted objects to a file, and then calculate the MessageDigest of the enciphered output. This test code is using try with resources block. When this block ends, the try block closure calls each of these in succession: oos.close(); cos.close(); dos.close(); fos.close(); oos.close call itself calls: cos.close(); // which calls doFinal(); dos.close(); // which calls calls fos.close(); The cos.close() is called again as part of the try block closure. It then causes doFinal() to be called again! This doFinal writes an empty but padded block to dos(), which screws up the MessageDigest that will eventually be digested. Fortunately, when dos.write() calls to fos.write(), which throws an exception that is swallowed, so the file doesn't contain that extra empty padded block. But any other potential downstream OutputStreams could be affected. MessageDigest is screwed up now. As for fixing in the DigestOutputStream as mentioned in the Description, I think that is probably the right thing to do also.
13-04-2012