| Message ID | 20210915080948.11891-3-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cvsserver: correctly validate pserver passwords | expand |
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > - if (crypt(descramble($password), $1) eq $1) { > - $auth_ok = 1; > + my $hash = crypt(descramble($password), $1); > + if (defined $hash) { > + $auth_ok = 1 if $hash eq $1; > } It is not wrong per-se to separate the two checks into two separate parts of the conditional, but because we check for definedness only because comparison of it with $1 makes sense only when it is defined, writing it either like this, if (defined $hash and $hash eq $1) { $auth_ok = 1; } or even like this, $auth_ok = (defined $hash and $hash eq $1); may be easier to read, no?
On Thu, Sep 16, 2021 at 3:11 PM Junio C Hamano <gitster@pobox.com> wrote: > > Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > > > - if (crypt(descramble($password), $1) eq $1) { > > - $auth_ok = 1; > > + my $hash = crypt(descramble($password), $1); > > + if (defined $hash) { > > + $auth_ok = 1 if $hash eq $1; > > } > > It is not wrong per-se to separate the two checks into two separate > parts of the conditional, but because we check for definedness only > because comparison of it with $1 makes sense only when it is > defined, writing it either like this, > > if (defined $hash and $hash eq $1) { > $auth_ok = 1; > } > > or even like this, > > $auth_ok = (defined $hash and $hash eq $1); > > may be easier to read, no? yes, let's go with the earlier; I was trying to mimic the original code, but agree on a second read that it looks confusing. assuming there are no more comments, would you want a reroll? Carlo
Carlo Arenas <carenas@gmail.com> writes: >> It is not wrong per-se to separate the two checks into two separate >> parts of the conditional, but because we check for definedness only >> because comparison of it with $1 makes sense only when it is >> defined, writing it either like this, >> >> if (defined $hash and $hash eq $1) { >> $auth_ok = 1; >> } >> >> or even like this, >> >> $auth_ok = (defined $hash and $hash eq $1); >> >> may be easier to read, no? > > yes, let's go with the earlier; I was trying to mimic the original > code, but agree on a second read that it looks confusing. > assuming there are no more comments, would you want a reroll? If there are no more comments, I certainly can tweak it myself, but I'd rather not have to keep an eye to see if the thread gets any comment myself to begin with, as it will not scale as the process when there are multiple contributors and only one maintainer. So, I'll make a local tweak myself and mark the topic as "On hold", to let you ping me after waiting for a while to see if the thread gets other comments that require a reroll. Thanks.
diff --git a/git-cvsserver.perl b/git-cvsserver.perl index 4c93b5d099..8e7c34a235 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -222,10 +222,11 @@ open my $passwd, "<", $authdb or die $!; while (<$passwd>) { if (m{^\Q$user\E:(.*)}) { - if (crypt(descramble($password), $1) eq $1) { - $auth_ok = 1; + my $hash = crypt(descramble($password), $1); + if (defined $hash) { + $auth_ok = 1 if $hash eq $1; } - }; + } } close $passwd; diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index 59b40359c7..17f988edd2 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -36,7 +36,12 @@ CVSWORK="$PWD/cvswork" CVS_SERVER=git-cvsserver export CVSROOT CVS_SERVER -PWDHASH='lac2ItudM3.KM' +if perl -e 'exit(1) if not defined crypt("", "cv")' +then + PWDHASH='lac2ItudM3.KM' +else + PWDHASH='$2b$10$t8fGvE/a9eLmfOLzsZme2uOa2QtoMYwIxq9wZA6aBKtF1Yb7FJIzi' +fi rm -rf "$CVSWORK" "$SERVERDIR" test_expect_success 'setup' '
Some versions of crypt(3) will return NULL when passed an unsupported hash type (ex: OpenBSD with DES), so check for undef instead of using it directly. Also use this to probe the system and select a better hash function in the tests, so it can pass successfully. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- git-cvsserver.perl | 7 ++++--- t/t9400-git-cvsserver-server.sh | 7 ++++++- 2 files changed, 10 insertions(+), 4 deletions(-)