[jira] [Commented] (FTPSERVER-486) Timing Side Channel StringUtils
[
https://issues.apache.org/jira/browse/FTPSERVER-486?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16684521#comment-16684521
]
Yannic Noller commented on FTPSERVER-486:
-
What is the current status of the issue?
-- Yannic
> Timing Side Channel StringUtils
> ---
>
> Key: FTPSERVER-486
> URL: https://issues.apache.org/jira/browse/FTPSERVER-486
> Project: FtpServer
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.1.1
> Environment: test on macOS High Sierra 10.13.4, but not relevant
>Reporter: Yannic Noller
>Assignee: Emmanuel Lecharny
>Priority: Major
> Labels: easyfix, pull-request-available
> Fix For: 1.1.2
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> Dear Apache FTPServer developers,
> We have found a timing side-channel in class
> org.apache.ftpserver.util.StringUtils, method "public final static String
> pad(String src, char padChar, boolean rightPad, int totalLength)". This
> method leaks the necessary padding in a timing side channel, from which a
> potential attacker could obtain the length of the src String. In your project
> this method is used to add padding to a username, hence, a potential attacker
> could obtain the length of a given username, which might be used for further
> attacks.
> Do you agree with our findings?
> We found this class in the latest version of your git repo:
> https://git-wip-us.apache.org/repos/asf?p=mina-ftpserver.git;a=summary
> As a secure fix we would recommend to use a variant of the equals method,
> which does iterate the complete strings in the case of the same string
> lengths, independent from whether they do match or not:
>public final static String pad_safe(String src, char padChar, boolean
> rightPad, int totalLength) {
>int srcLength = src.length();
>if (srcLength >= totalLength) {
>return src;
>}
>int padLength = totalLength - srcLength;
>StringBuilder sb = new StringBuilder(padLength);
>for (int i = 0; i < totalLength; ++i) {
>if (i < padLength) {
>sb.append(padChar);
>} else {
>sb.append("");
>}
>}
>if (rightPad) {
>return src + sb.toString();
>} else {
>return sb.toString() + src;
>}
>}
> Do you agree with our patch proposal?
> Please feel free to contact us for further clarification! You can reach us by
> the following email address:
> yannic.nol...@informatik.hu-berlin.de
> Best regards,
> Yannic Noller
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FTPSERVER-486) Timing Side Channel StringUtils
[
https://issues.apache.org/jira/browse/FTPSERVER-486?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16464704#comment-16464704
]
Yannic Noller commented on FTPSERVER-486:
-
Hi Emmanuel,
thanks for looking into this issue. I double-checked my proposed fix, and came
up with a new one:
public final static String pad_safe(String src, char padChar, boolean
rightPad, int totalLength) {
int srcLength = Math.min(src.length(), totalLength);
int padLength = totalLength - srcLength;
StringBuilder sb = new StringBuilder(padLength);
StringBuilder sb_fake = new StringBuilder(srcLength);
for (int i = 0; i < totalLength; ++i) {
if (i < padLength) {
sb.append(padChar);
} else {
sb_fake.append(padChar);
}
}
String returnString;
if (rightPad) {
returnString = src + sb.toString();
} else {
returnString = sb.toString() + src;
}
return returnString;
}
Thanks,
> Timing Side Channel StringUtils
> ---
>
> Key: FTPSERVER-486
> URL: https://issues.apache.org/jira/browse/FTPSERVER-486
> Project: FtpServer
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.1.1
> Environment: test on macOS High Sierra 10.13.4, but not relevant
>Reporter: Yannic Noller
>Assignee: Emmanuel Lecharny
>Priority: Major
> Labels: easyfix, pull-request-available
> Fix For: 1.1.2
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> Dear Apache FTPServer developers,
> We have found a timing side-channel in class
> org.apache.ftpserver.util.StringUtils, method "public final static String
> pad(String src, char padChar, boolean rightPad, int totalLength)". This
> method leaks the necessary padding in a timing side channel, from which a
> potential attacker could obtain the length of the src String. In your project
> this method is used to add padding to a username, hence, a potential attacker
> could obtain the length of a given username, which might be used for further
> attacks.
> Do you agree with our findings?
> We found this class in the latest version of your git repo:
> https://git-wip-us.apache.org/repos/asf?p=mina-ftpserver.git;a=summary
> As a secure fix we would recommend to use a variant of the equals method,
> which does iterate the complete strings in the case of the same string
> lengths, independent from whether they do match or not:
>public final static String pad_safe(String src, char padChar, boolean
> rightPad, int totalLength) {
>int srcLength = src.length();
>if (srcLength >= totalLength) {
>return src;
>}
>int padLength = totalLength - srcLength;
>StringBuilder sb = new StringBuilder(padLength);
>for (int i = 0; i < totalLength; ++i) {
>if (i < padLength) {
>sb.append(padChar);
>} else {
>sb.append("");
>}
>}
>if (rightPad) {
>return src + sb.toString();
>} else {
>return sb.toString() + src;
>}
>}
> Do you agree with our patch proposal?
> Please feel free to contact us for further clarification! You can reach us by
> the following email address:
> yannic.nol...@informatik.hu-berlin.de
> Best regards,
> Yannic Noller
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (FTPSERVER-486) Timing Side Channel StringUtils
[
https://issues.apache.org/jira/browse/FTPSERVER-486?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16440862#comment-16440862
]
Emmanuel Lecharny commented on FTPSERVER-486:
-
Hi,
this seems to be a potential leak of information, yes. I think we can do better
:
o iterate on each char from the original string, appending each char to the
resulting string, then complete with as many char needed to pad the result (or
pad first, depending on the padding position)
Note that it can be improved by creating a pad string that will have the final
string length, and iterate on this pad string, to be sure the {{charAt(pos)}}
is called for both teh padding string and the original string (because not
calling {{charAt(pos)}} while padding leaks a bit of information too).
Your proposed patch still leak some timing information :
{{StringBuilder.append( "" )}} will not be executed in the same time than
{{StringBuilder.append( char )}}, because in no way the {{StringBuilder}} will
be reallocated when the limit will be reached in youc all it with an empty
\{{String}}.
> Timing Side Channel StringUtils
> ---
>
> Key: FTPSERVER-486
> URL: https://issues.apache.org/jira/browse/FTPSERVER-486
> Project: FtpServer
> Issue Type: Bug
> Components: Core
>Affects Versions: 1.1.1
> Environment: test on macOS High Sierra 10.13.4, but not relevant
>Reporter: Yannic Noller
>Assignee: Emmanuel Lecharny
>Priority: Major
> Labels: easyfix, pull-request-available
> Fix For: 1.1.2
>
> Original Estimate: 24h
> Remaining Estimate: 24h
>
> Dear Apache FTPServer developers,
> We have found a timing side-channel in class
> org.apache.ftpserver.util.StringUtils, method "public final static String
> pad(String src, char padChar, boolean rightPad, int totalLength)". This
> method leaks the necessary padding in a timing side channel, from which a
> potential attacker could obtain the length of the src String. In your project
> this method is used to add padding to a username, hence, a potential attacker
> could obtain the length of a given username, which might be used for further
> attacks.
> Do you agree with our findings?
> We found this class in the latest version of your git repo:
> https://git-wip-us.apache.org/repos/asf?p=mina-ftpserver.git;a=summary
> As a secure fix we would recommend to use a variant of the equals method,
> which does iterate the complete strings in the case of the same string
> lengths, independent from whether they do match or not:
>public final static String pad_safe(String src, char padChar, boolean
> rightPad, int totalLength) {
>int srcLength = src.length();
>if (srcLength >= totalLength) {
>return src;
>}
>int padLength = totalLength - srcLength;
>StringBuilder sb = new StringBuilder(padLength);
>for (int i = 0; i < totalLength; ++i) {
>if (i < padLength) {
>sb.append(padChar);
>} else {
>sb.append("");
>}
>}
>if (rightPad) {
>return src + sb.toString();
>} else {
>return sb.toString() + src;
>}
>}
> Do you agree with our patch proposal?
> Please feel free to contact us for further clarification! You can reach us by
> the following email address:
> yannic.nol...@informatik.hu-berlin.de
> Best regards,
> Yannic Noller
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
