commons-codec icon indicating copy to clipboard operation
commons-codec copied to clipboard

Base64 should be open for different alphabets

Open cljk opened this issue 5 years ago • 7 comments

Currently I´m trying to write a Linux shadow-file - the passwords are stored as Base64-ecoded (hashes). But the alphabet of this Base64 is different:

see www.linuxquestions.org/questions/linux-security-4/how-can-i-convert-a-sha-512-etc-shadow-hash-to-base64-4175477045/ A base 64 encoding is used to store password hashes computed with crypt in the /etc/passwd. Its alphabet starts with '.' for zero, then '/' for one, followed by 0-9, A-Z and a-z. Padding is not used.

So I opened up your Base64 class constructor to accept an alphabet.

cljk avatar Jul 23 '19 11:07 cljk

Coverage Status

Coverage decreased (-0.04%) to 92.74% when pulling c44f596cdd942ec73011581b708d752c54fa1ec8 on cljk:master into 47a55d21515bf2ec49d2c1b4f2f83bf66a09a7c5 on apache:master.

coveralls avatar Jul 23 '19 11:07 coveralls

Coverage Status

Coverage increased (+0.02%) to 92.803% when pulling 3e0d13c035021da7921cae9702194bb0699f5071 on cljk:master into 47a55d21515bf2ec49d2c1b4f2f83bf66a09a7c5 on apache:master.

coveralls avatar Jul 23 '19 11:07 coveralls

You´re right. I added one unit test to check the encoding output of using a custom alphabet by comparing it to the default alphabet output.

I made the byte arrays of the alphabets public. Not just because I needed it for the unit test but I think that the encoding table might be necessary elsewhere in other projects?! If this is not wanted I´ll copy the array to the test-case...

When my personal project with the shadow table is finished I´d like to add the crypt table.

cljk avatar Jul 23 '19 20:07 cljk

Hm.... I´ve no idea why the travis build fails.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 58.236 s
[INFO] Finished at: 2019-07-23T20:05:09Z
[INFO] Final Memory: 47M/160M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.1.0:javadoc (default-cli) on project commons-codec: An error has occurred in Javadoc report generation: 
[ERROR] Exit code: 1 - javadoc: error - The code being documented uses modules but the packages defined in https://docs.oracle.com/javase/7/docs/api/ are in the unnamed module.
[ERROR] javadoc: error - The code being documented uses modules but the packages defined in https://docs.oracle.com/javaee/6/api/ are in the unnamed module.
[ERROR] javadoc: error - The code being documented uses modules but the packages defined in https://docs.oracle.com/javase/7/docs/api/ are in the unnamed module.
[ERROR] 
[ERROR] Command line was: /home/travis/oraclejdk11/bin/javadoc @options @packages
[ERROR] 
[ERROR] Refer to the generated Javadoc files in '/home/travis/build/apache/commons-codec/target/site/apidocs' dir.
[ERROR] 

I work on Corretto JDK 11 and my maven build succeeds?!

mvn clean package site

works on my system.

cljk avatar Jul 23 '19 20:07 cljk

Okay... can reproduce the error - but it´s no fault in my changes. Same error occurs in original code.

cljk avatar Jul 23 '19 20:07 cljk

I think the build fails because the maven-javadoc-plugin configuration section in build does not have this:

<source>${maven.compiler.source}</source>

The plugin is configured in commons-parent. It has this tag for the reporting section and the release profile but not for the build section.

@cljk Can you confirm that you cannot do this with Corretto JDK 11:

mvn javadoc:javadoc

or default goal

mvn

This should work:

mvn javadoc:javadoc -Dsource=1.7

On the code change, do not make the array public. The values are mutable. Copy the array to the unit test or provide a package private level static getter which returns a clone of the array.

You should also change the public constructor that accepts an input alphabet to check it has 64 characters. I'm not sure where we are with defensive copying of input arrays so I'll leave that judgement for someone else.

aherbert avatar Jul 23 '19 21:07 aherbert

@aherbert Yes, I can confirm:

mvn javadoc:javadoc

fails, mvn too and

mvn javadoc:javadoc -Dsource=1.7

works...

But perhaps we should open an issue for that since that´s not caused by my pull request?!


  • added check for encodeTable length ... and a unit test for this check
  • encode tables private again
  • reintruduced(?) usage of local variable decodeTable - needed for successful decoding of data encoded with custom encoding table
    • for STANDARD_ENCODE_TABLE or URL_SAFE_ENCODE_TABLE it uses the precalculated DEFAULT_DECODE_TABLE as before
    • for custom encode tables: added a calculation method for decode tables ... and an encode/decode test for a modified encode table

" defensive copying of input arrays" - If you think making a duplicate of the encodeTable improves security in any way (?)... adding this would be a one-liner?!

cljk avatar Jul 23 '19 22:07 cljk