[
https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16718803#comment-16718803
]
Goldstein Lyor commented on SSHD-708:
-------------------------------------
Hi [~wolft],
Sorry about the PR, but I thought everything is in order since all tests passed
- including the ones with the encryption.
{quote}
Thanks for the attribution, but it wouldn't have been necessary. Most of the
code I had written is not in that commit anyway but in the parent commit.
{quote}
I usually like to acknowledge those who contributed to the code since it is a
*community* effort.
{quote}
It's broken. MAX_ROUNDS is wrong. OpenSSH does not store an exponent for the
rounds but the number of rounds. I even had a comment in my code pointing this
out. The code as is will fail to decode a key generated with ssh-keygen -t
ed25519 -a 33. Besides, I don't understand the comment about "doubling". What
doubles?
{quote}
As I was reading the {{BCrypt.java}} code and its comments I came across the
following:
{quote}
The amount of work increases exponentially (2**log_rounds), so each increment
is twice as much work. The default log_rounds is 10, and the valid range is 4
to 30.
{quote}
and also this
{code:java}
public byte[] crypt_raw(byte password[], byte salt[], int log_rounds,
int cdata[]) {
int rounds, i, j;
int clen = cdata.length;
byte ret[];
if (log_rounds < 4 || log_rounds > 30)
throw new IllegalArgumentException ("Bad number of rounds");
{code}
This is what (mis-)led me to set a {{MAX_ROUNDS = 31}}. I see now that I was
looking at the wrong location since {{pbkdf}} uses it in a simple loop:
{code:java}
for (int round = 1; round < rounds; round++) {
sha512.reset();
sha512.update(tmp);
sha512.digest(hsalt, 0, hsalt.length);
....
}
{code}
What I am trying to do is prevent some kind of "attack" by providing a
malicious (or corrupted) value that would cause the code to "hang" by executing
a very large number of round (e.g., what if someone uses 2^31 rounds). While
the value is defined as {{unit32}} I would like to impose a reasonable limit
value on it so when we decode the KDF options we will know that it is wrong.
There are 2 options that I would value your opinion:
# Define a large hardcoded value to accommodate all reasonable values - 1024 ?
2048 ? 4096 ? ... ?
# Do the above as *default value*, but add a system property and/or session
property to allow users to define a different value.
{quote}
Setting local variables to null at the end of methods doesn't improve "getting
rid of sensitive data", does it? For arrays, yes, clearing the values should be
done, but if a password comes in as a String, there's not much one can do.
{quote}
In essence you are right, however, the variable is holding a reference to the
object - by null-ifying it I am hinting to the GC that it can be disposed of.
Especially the password that is part of a loop. I agree that there are no
guarantees, but in the spirit of zero-ing key related data a.s.a.p. I don't see
the harm even if there is no GC.
{quote}
This constructor will bypass the length check on the salt in Line 81. Since
this constructor is never used, why add it at all?
{quote}
Agreed - I actually wrote it before incorporating your code and forgot to
remove it - will do so.
{quote}
bcryptKdf doesn't throw any checked exceptions. The catch block you added above
should actually be inside bcryptKdf, and it should not handle IOException
(never thrown there), only GeneralSecurityException. This should simply be as
in my PR.
{quote}
The code is *generic* - i.e., if we ever replace the {{bcrypt}} with something
else, this is how the I would like the {{catch}} block to behave, so I don't
see the harm in writing the code in this manner now rather than later.
{quote}
Otherwise it's very hard to review this because the code has changed very much,
has become IMO needlessly general and complicated, and the individual commits
include unrelated formatting changes.
{quote}
You are right in essence - sometimes I include formatting changes as part of
the commit since I don't want to use a separate commit only for them. I try to
resist this tendency, but sometimes I just can't help it ... :-).
I will publish a PR on the changes once done.
> Add support for password encrypted OpenSSH private key files
> ------------------------------------------------------------
>
> Key: SSHD-708
> URL: https://issues.apache.org/jira/browse/SSHD-708
> Project: MINA SSHD
> Issue Type: Improvement
> Affects Versions: 1.4.0
> Reporter: Goldstein Lyor
> Assignee: Goldstein Lyor
> Priority: Minor
> Fix For: 2.1.1
>
>
> The current code supports only reading un-encrypted private key files
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
