Mike, The issue you are describing exists both before and after Marks proposed patch. I am somewhat sympathetic to it, but it is outside the scope of what this issue is trying to address.
Since the code is open ;-) you can of course propose a separate patch, but I don't think it should prevent this proceeding. -Chris On 15 Feb 2013, at 20:26, Michael StJohns <mstjo...@comcast.net> wrote: > At 11:57 AM 2/15/2013, Chris Hegarty wrote: >> Mike, >> >> I believe that Marks changes are a like for like replacement with what was >> there before, only using a supported public API. Are you saying otherwise? >> Or have you identified another potential issue? > > I ran into this problem when trying to do my own de-armoring code and dealing > with stuff that had been OCR'd - (don't ask -it was painful all around, but > it was what I had). I ended up having to wrap the de-armoring code in regex > processing to ensure well-formed base64. > > I guess what I'm saying is that you should be able to detect a base64 problem > and report it before trying to decode the byte stream. The fact that the old > code didn't do this is occasionally problematic as I have at times spent > hours trying to figure out what was wrong with the certificate encoding when > it was the base64 that was confused. > > While I'm at it ( :-) ) it would be nice the code that detects the sentinels > (e.g. the "-----BEGIN <etc>-----") would accept a number of the more common > variations - "BEGIN X509 CERTIFICATE" "BEGIN CERTIFICATE REQUEST" (instead > of "BEGIN CERTIFICATE and BEGIN NEW CERTIFICATE..."), and not care about > white space between the "-----" and the beginning or end of the sentinel > string. > > To answer the question you didn't quite ask - no, this isn't critical, but > since the code is open... just a thought. > > Mike > > > >> -Chris. >> >> On 15/02/2013 16:52, Michael StJohns wrote: >>> Is the "mime" variant of Base64 the correct one for this? I ask because >>> that variant ignores extraneous characters rather than throwing an error on >>> decode. Also, reading the code for the Base64 implementation, it silently >>> "fixes" the case where there are missing padding "=" characters. Neither >>> of these seem ideal for security related processing. >>> >>> It may be reasonable to add a PEM variant to the Base64 code that deals >>> with the above. >>> >>> Mike >>> >>> >>> >>> At 08:24 AM 2/14/2013, Mark Sheppard wrote: >>>> Hi, >>>> as part of a refactoring of the jdk codebase to use the base64 >>>> capabilities of java.util.Base64, the following modifications, >>>> as per the webrev, >>>> >>>> http://cr.openjdk.java.net/~chegar/8006182/webrev.00/ >>>> >>>> have been made to complete task JDK-8006182. >>>> >>>> Could you oblige and review these changes, please? >>>> >>>> Description: >>>> jdk8 has java.util.Base64 to define a standard API for base64 >>>> encoding/decoding. It would be good to investigate whether this API could >>>> be used in the security components, providers and regression tests. >>>> >>>> In the main this work involved replacing the sun.misc.BASE64Encoder and >>>> sun.misc.BASE64Decoder with the >>>> corresponding Mime Base64 Encoder/Decoder (as per rfc2045) from the >>>> java.util.Base64 class. >>>> This is a like for like replacement. >>>> As such, sun.misc.BASE64Encoder maps to the encoder returned by >>>> java.util.Base64.getMimeEncoder() >>>> sun.misc.BASE64Decoder maps to the decoder returned by >>>> java.util.Base64.getMimeDecoder() >>>> >>>> However a couple of items worth noting: >>>> >>>> In the jarsigner (Main.java) the standard Base64 encoder (rfc 4648), >>>> java.util.Base64.getEncoder(), has been used to replace the >>>> JarBASE64Encoder, which was a package private extension of BASE64Encoder, >>>> which avoids writing newline to the encoded data. >>>> >>>> In the keytool (Main.java), methods such as dumpCert, printCert. printCRL, >>>> and so on, write a Base64 encoding to an OutputStream, typically std out. >>>> This is achieved in the BASE64Encoder, by passing the OutputStream to >>>> methods such as encodeBuffer(). >>>> >>>> A couple of options exist to do this under the new Base64 utilities, which >>>> include: >>>> >>>> * using a Mime Encoder encodeToString() and output to the stream via >>>> println() >>>> >>>> * use the wrap capabilities of the Base64.Encoder: >>>> - define a package private class, which extends FilterOutputStream (e.g. >>>> NoCloseWrapperOutputStream) and, overrides close() to do nothing >>>> - inject the OutputStream, passed to the keytool method, into the >>>> NoCloseWrapperOutputStreamwapper, >>>> - wrap() the NoCloseWrapperOutputStreamwrapper in the Mime Encoder, >>>> which will in turn return an encapsulating OutputStream; >>>> - write the data buffer to be encoded to the encoder's OutputStream; >>>> - close the encoder's OutputStream, which completes the base64 encoding; >>>> - append a newline to the initial OutputStream. >>>> >>>> pragmatics and the simplest thing that works, went for the first option. >>>> >>>> regards >>>> Mark > >
