Marco Trudel wrote:
> I fixed some stuff in javax.crypto:
>
Thanks for looking at this. Some of these parts of javax.crypto were
badly implemented, and I have to claim responsibility for that.
> 1. decryption with padding was broken/wrong handled
> 2. CipherOutputStream was completly broken/unusuable
> 3. PKCS7 did an unnecessary test
>
> 1. decryption with padding needs to keep back the last block for final
> unpadding when doFinal() is called. This wasn't done. doFinal() lead to
> an exception.
> Actually, padded decrypting only worked correct when all data was passed
> by doFinal(byte[]) or when there where update() calls that filled the
> data to a multiple of the blocklengh and doFinal was called with the
> rest of the data.
> This fixes CiperInputStream as well, because it relies on the correct
> doFinal() handling of the cipher class.
>
Understood. Sounds fine.
> 2. CipherOutputstream had a lot of code that did nothing except leading
> to a NullPointerException when calling write(...) (outBuffer was never
> initialized). It looks to me like the code should have worked around the
> bugs in CipherAdapter. But that should have been done in
> CipherInputStream?!
>
I don't know. I wrote all of these, so all I can do is claim ignorance
on how this stuff was supposed to work :-\
> 3. PKCS7 unpadding tested a value that was just read with itself. Fixed
> it because I was already reading it... Nothing big...
>
OK.
> Any comments? Hints?
>
It certainly looks more correct, to my eyes. I haven't tried this out
yet, but it looks much better, and certainly simpler.
> I have no committing rights and the copyright assignment papers are not
> yet arrived with mail. This might be a problem...
>
Yeah, we won't be able to accept this patch until you have a copyright
assignment on file. But if it is your intention to contribute this, then
it's just a matter of time.
Have we gotten you the correct forms? Maybe Mark can tell if this
process is moving forward or not.
> @@ -447,16 +459,20 @@
> break;
> case IMode.DECRYPTION:
> int padLen;
> + byte[] buf3 = new byte[buf.length + partLen];
> try
> {
> - padLen = pad.unpad(buf, 0, buf.length);
> + if(partLen != 16) throw new WrongPaddingException();
What is this constant? Does this work for all block sizes and all
padding schemes?
[...]
> + {
> + out.write(cipher.doFinal());
> + out.flush();
> + out.close();
> + } catch (javax.crypto.IllegalBlockSizeException ibse)
> + {
> + throw new IOException(ibse.toString());
> + } catch (javax.crypto.BadPaddingException bpe)
> + {
> + throw new IOException(bpe.toString());
> + }
It would be better to chain the exceptions here, and to omit the
explicit 'javax.crypto' package references (I know, they were in the
original).
