[jira] [Comment Edited] (LOG4J2-3056) NameUtil.md5(String) could leak plaintext credentials

["Ralph Goers (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/LOG4J2-3056?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17310077#comment-17310077 ] 

Ralph Goers edited comment on LOG4J2-3056 at 3/27/21, 8:23 PM:
---------------------------------------------------------------

I would first check to see what we are using the md5 method for. Sometimes it is just used to get a fixed size String as a way to represent the value, in which case this issue is irrelevant. If it is being used to encode something that needs to be secure we should be using a different algorithm. MD5 is not considered secure.

 
 # The Flume Appender uses it to generate an agent name. (Not a problem).
 # PluginAttributeVisitor uses it when the PluginAttribute's sensitive attribute is set true. I'm not sure why we don't just mask it with asterisks. It should not use md5.
 # PluginBuilderAttributeVisitor - same as PluginAttributeVisitor.
 # CouchDb - used to mask the connection password in the toString method of the provider. Should use something more secure.
 # MongoDb - same as CouchDb. 


was (Author: ralph.goers@dslextreme.com):
I would first check to see what we are using the md5 method for. Sometimes it is just used to get a fixed size String as a way to represent the value, in which case this issue is irrelevant. If it is being used to encode something that needs to be secure we should be using a different algorithm. MD5 is not considered secure.

> NameUtil.md5(String) could leak plaintext credentials
> -----------------------------------------------------
>
>                 Key: LOG4J2-3056
>                 URL: https://issues.apache.org/jira/browse/LOG4J2-3056
>             Project: Log4j 2
>          Issue Type: Bug
>            Reporter: Marcono1234
>            Assignee: Volkan Yazici
>            Priority: Major
>             Fix For: 2.15.0
>
>
> {{org.apache.logging.log4j.util.NameUtil.md5(String)}} could leak the credentials provided to it in case an exception is thrown:
> {code}
> public static String md5(final String string) {
>     try {
>         ...
>     } catch (final Exception ex) {
>         return string; // leaks plaintext credentials
>     }
> }
> {code}
> This is however very likely not a security issue currently because it appears the only exception which could occur is {{NoSuchAlgorithmException}}.
> Nonetheless I would recommend the following changes:
> - *Never return the plaintext {{String}}*
> - Wrap the {{getInstance("MD5")}} call with a {{try-catch}} catching the {{NoSuchAlgorithmException}}, wrapping it in an {{AssertionError}} and throw that, since the [documentation for {{MessageDigest}}|https://docs.oracle.com/javase/8/docs/api/java/security/MessageDigest.html] guarantees that every JRE must support MD5:
> {code}
> MessageDigest md5;
> try {
>     md5 = MessageDigest.getInstance("MD5");
> } catch (NoSuchAlgorithmException e) {
>     // Impossible; MessageDigest documentation guarantees that MD5 is supported
>     throw new AssertionError("MD5 is not supported", e);
> }
> {code}
> - Don't use {{string.getBytes()}}, that uses the default charset of the platform. I don't know what the 'correct' charset here would be (maybe UTF-8?), but it is very likely not the default charset.
> - Optional: Omit the call to {{MessageDigest.update}} and instead only call {{digest(byte[])}}.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)