{"thread":{"id":"56501","subject":"[PATCH 1/3] git-cvsserver: use crypt correctly to compare password hashes","startedAt":"2021-09-15T08:11:00Z","lastAt":"2021-09-17T03:43:35Z","messageCount":7,"participants":["Carlo Marcelo Arenas Belón","Junio C Hamano","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"435967","messageId":"20210915080948.11891-2-carenas@gmail.com","threadId":"56501","inReplyTo":"20210915080948.11891-1-carenas@gmail.com","subject":"[PATCH 1/3] git-cvsserver: use crypt correctly to compare password hashes","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-15T08:09:46Z","receivedAt":"2021-09-15T08:11:00Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"c057bad370 (git-cvsserver: use a password file cvsserver pserver,\n2010-05-15) adds a way for `git cvsserver` to provide authenticated\npserver accounts without having clear text passwords, but uses the\nusername instead of the password to the call for crypt(3).\n\nCorrect that, and make sure the documentation correctly indicates how\nto obtain hashed passwords that could be used to populate this\nconfiguration, as well as correcting the hash that was used for the\ntests.\n\nThis change will require that any user of this feature updates the\nhashes in their configuration, but has the advantage of using a more\nsimilar format than cvs uses, probably also easying any migration.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n Documentation/git-cvsserver.txt | 10 ++++------\n git-cvsserver.perl              |  2 +-\n t/t9400-git-cvsserver-server.sh |  4 +++-\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt\nindex f2e4a47ebe..4d13367c77 100644\n--- a/Documentation/git-cvsserver.txt\n+++ b/Documentation/git-cvsserver.txt\n@@ -118,13 +118,11 @@ for example:\n    myuser:$1$BA)@$vbnMJMDym7tA32AamXrm./\n ------\n You can use the 'htpasswd' facility that comes with Apache to make these\n-files, but Apache's MD5 crypt method differs from the one used by most C\n-library's crypt() function, so don't use the -m option.\n+files, but only with the -d option (or -B if your system suports it).\n \n-Alternatively you can produce the password with perl's crypt() operator:\n------\n-   perl -e 'my ($user, $pass) = @ARGV; printf \"%s:%s\\n\", $user, crypt($user, $pass)' $USER password\n------\n+Preferably use the system specific utility that manages password hash\n+creation in your platform (e.g. mkpasswd in Linux, encrypt in OpenBSD or\n+pwhash in NetBSD) and paste it in the right location.\n \n Then provide your password via the pserver method, for example:\n ------\ndiff --git a/git-cvsserver.perl b/git-cvsserver.perl\nindex ed035f32c2..4c93b5d099 100755\n--- a/git-cvsserver.perl\n+++ b/git-cvsserver.perl\n@@ -222,7 +222,7 @@\n         open my $passwd, \"<\", $authdb or die $!;\n         while (<$passwd>) {\n             if (m{^\\Q$user\\E:(.*)}) {\n-                if (crypt($user, descramble($password)) eq $1) {\n+                if (crypt(descramble($password), $1) eq $1) {\n                     $auth_ok = 1;\n                 }\n             };\ndiff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\nindex 2d29d486ee..59b40359c7 100755\n--- a/t/t9400-git-cvsserver-server.sh\n+++ b/t/t9400-git-cvsserver-server.sh\n@@ -36,6 +36,8 @@ CVSWORK=\"$PWD/cvswork\"\n CVS_SERVER=git-cvsserver\n export CVSROOT CVS_SERVER\n \n+PWDHASH='lac2ItudM3.KM'\n+\n rm -rf \"$CVSWORK\" \"$SERVERDIR\"\n test_expect_success 'setup' '\n   git config push.default matching &&\n@@ -54,7 +56,7 @@ test_expect_success 'setup' '\n   GIT_DIR=\"$SERVERDIR\" git config --bool gitcvs.enabled true &&\n   GIT_DIR=\"$SERVERDIR\" git config gitcvs.logfile \"$SERVERDIR/gitcvs.log\" &&\n   GIT_DIR=\"$SERVERDIR\" git config gitcvs.authdb \"$SERVERDIR/auth.db\" &&\n-  echo cvsuser:cvGVEarMLnhlA > \"$SERVERDIR/auth.db\"\n+  echo \"cvsuser:$PWDHASH\" >\"$SERVERDIR/auth.db\"\n '\n \n # note that cvs doesn't accept absolute pathnames\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"435968","messageId":"20210915080948.11891-1-carenas@gmail.com","threadId":"56501","inReplyTo":null,"subject":"[PATCH 0/3] cvsserver: correctly validate pserver passwords","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-15T08:09:45Z","receivedAt":"2021-09-15T08:11:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"The first patch should be applied by any user of git-cvsserver, and\nhashes for all pserver accounts updated; because the code originally\nwas using the username instead of the password to validate accounts,\nand it might had even worked if the first 2 characters of the password\nwhere the same (ex: cvsuser/cvspassword).\n\nThe second one allows for successfully running t9400 in OpenBSD and\nwill protect the code further from the possible use of an undef\nvariable, and shows that support for better password hashes than DES\nis possible.\n\nCarlo Marcelo Arenas Belón (3):\n  git-cvsserver: use crypt correctly to compare password hashes\n  git-cvsserver: protect against NULL in crypt(3)\n  Documentation: cleanup git-cvsserver\n\n Documentation/git-cvsserver.txt | 27 +++++++++++++--------------\n git-cvsserver.perl              |  7 ++++---\n t/t9400-git-cvsserver-server.sh |  9 ++++++++-\n 3 files changed, 25 insertions(+), 18 deletions(-)\n\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"435969","messageId":"20210915080948.11891-3-carenas@gmail.com","threadId":"56501","inReplyTo":"20210915080948.11891-1-carenas@gmail.com","subject":"[PATCH 2/3] git-cvsserver: protect against NULL in crypt(3)","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-15T08:09:47Z","receivedAt":"2021-09-15T08:11:24Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Some versions of crypt(3) will return NULL when passed an unsupported\nhash type (ex: OpenBSD with DES), so check for undef instead of using\nit directly.\n\nAlso use this to probe the system and select a better hash function in\nthe tests, so it can pass successfully.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n git-cvsserver.perl              | 7 ++++---\n t/t9400-git-cvsserver-server.sh | 7 ++++++-\n 2 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/git-cvsserver.perl b/git-cvsserver.perl\nindex 4c93b5d099..8e7c34a235 100755\n--- a/git-cvsserver.perl\n+++ b/git-cvsserver.perl\n@@ -222,10 +222,11 @@\n         open my $passwd, \"<\", $authdb or die $!;\n         while (<$passwd>) {\n             if (m{^\\Q$user\\E:(.*)}) {\n-                if (crypt(descramble($password), $1) eq $1) {\n-                    $auth_ok = 1;\n+                my $hash = crypt(descramble($password), $1);\n+                if (defined $hash) {\n+                    $auth_ok = 1 if $hash eq $1;\n                 }\n-            };\n+            }\n         }\n         close $passwd;\n \ndiff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh\nindex 59b40359c7..17f988edd2 100755\n--- a/t/t9400-git-cvsserver-server.sh\n+++ b/t/t9400-git-cvsserver-server.sh\n@@ -36,7 +36,12 @@ CVSWORK=\"$PWD/cvswork\"\n CVS_SERVER=git-cvsserver\n export CVSROOT CVS_SERVER\n \n-PWDHASH='lac2ItudM3.KM'\n+if perl -e 'exit(1) if not defined crypt(\"\", \"cv\")'\n+then\n+\tPWDHASH='lac2ItudM3.KM'\n+else\n+\tPWDHASH='$2b$10$t8fGvE/a9eLmfOLzsZme2uOa2QtoMYwIxq9wZA6aBKtF1Yb7FJIzi'\n+fi\n \n rm -rf \"$CVSWORK\" \"$SERVERDIR\"\n test_expect_success 'setup' '\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"435970","messageId":"20210915080948.11891-4-carenas@gmail.com","threadId":"56501","inReplyTo":"20210915080948.11891-1-carenas@gmail.com","subject":"[PATCH 3/3] Documentation: cleanup git-cvsserver","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-15T08:09:48Z","receivedAt":"2021-09-15T08:11:27Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Fix a few typos and alignment issues, and while at it update the\nexample hashes to show most of the ones available in recent crypt(3).\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n Documentation/git-cvsserver.txt | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt\nindex 4d13367c77..4dc57ed254 100644\n--- a/Documentation/git-cvsserver.txt\n+++ b/Documentation/git-cvsserver.txt\n@@ -99,7 +99,7 @@ looks like\n \n ------\n \n-Only anonymous access is provided by pserve by default. To commit you\n+Only anonymous access is provided by pserver by default. To commit you\n will have to create pserver accounts, simply add a gitcvs.authdb\n setting in the config file of the repositories you want the cvsserver\n to allow writes to, for example:\n@@ -114,8 +114,9 @@ The format of these files is username followed by the encrypted password,\n for example:\n \n ------\n-   myuser:$1Oyx5r9mdGZ2\n-   myuser:$1$BA)@$vbnMJMDym7tA32AamXrm./\n+   myuser:sqkNi8zPf01HI\n+   myuser:$1$9K7FzU28$VfF6EoPYCJEYcVQwATgOP/\n+   myuser:$5$.NqmNH1vwfzGpV8B$znZIcumu1tNLATgV2l6e1/mY8RzhUDHMOaVOeL1cxV3\n ------\n You can use the 'htpasswd' facility that comes with Apache to make these\n files, but only with the -d option (or -B if your system suports it).\n@@ -126,7 +127,7 @@ pwhash in NetBSD) and paste it in the right location.\n \n Then provide your password via the pserver method, for example:\n ------\n-   cvs -d:pserver:someuser:somepassword <at> server/path/repo.git co <HEAD_name>\n+   cvs -d:pserver:someuser:somepassword@server:/path/repo.git co <HEAD_name>\n ------\n No special setup is needed for SSH access, other than having Git tools\n in the PATH. If you have clients that do not accept the CVS_SERVER\n@@ -136,7 +137,7 @@ Note: Newer CVS versions (>= 1.12.11) also support specifying\n CVS_SERVER directly in CVSROOT like\n \n ------\n-cvs -d \":ext;CVS_SERVER=git cvsserver:user@server/path/repo.git\" co <HEAD_name>\n+   cvs -d \":ext;CVS_SERVER=git cvsserver:user@server/path/repo.git\" co <HEAD_name>\n ------\n This has the advantage that it will be saved in your 'CVS/Root' files and\n you don't need to worry about always setting the correct environment\n@@ -184,8 +185,8 @@ allowing access over SSH.\n +\n --\n ------\n-     export CVSROOT=:ext:user@server:/var/git/project.git\n-     export CVS_SERVER=\"git cvsserver\"\n+   export CVSROOT=:ext:user@server:/var/git/project.git\n+   export CVS_SERVER=\"git cvsserver\"\n ------\n --\n 4. For SSH clients that will make commits, make sure their server-side\n@@ -201,7 +202,7 @@ allowing access over SSH.\n    `project-master` directory:\n +\n ------\n-     cvs co -d project-master master\n+   cvs co -d project-master master\n ------\n \n [[dbbackend]]\n-- \n2.33.0.481.g26d3bed244\n\n"},{"id":"436175","messageId":"xmqqczp8rx1c.fsf@gitster.g","threadId":"56501","inReplyTo":"20210915080948.11891-3-carenas@gmail.com","subject":"Re: [PATCH 2/3] git-cvsserver: protect against NULL in crypt(3)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-16T22:11:11Z","receivedAt":"2021-09-16T22:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> -                if (crypt(descramble($password), $1) eq $1) {\n> -                    $auth_ok = 1;\n> +                my $hash = crypt(descramble($password), $1);\n> +                if (defined $hash) {\n> +                    $auth_ok = 1 if $hash eq $1;\n>                  }\n\nIt is not wrong per-se to separate the two checks into two separate\nparts of the conditional, but because we check for definedness only\nbecause comparison of it with $1 makes sense only when it is\ndefined, writing it either like this, \n\n\t\tif (defined $hash and $hash eq $1) {\n\t\t\t$auth_ok = 1;\n\t\t}\n\nor even like this,\n\n\t\t$auth_ok = (defined $hash and $hash eq $1);\n\nmay be easier to read, no?\n"},{"id":"436183","messageId":"CAPUEspjqD5zy8TLuFA96usU7FYi=0wF84y7NgOVFqegtxL9zbw@mail.gmail.com","threadId":"56501","inReplyTo":"xmqqczp8rx1c.fsf@gitster.g","subject":"Re: [PATCH 2/3] git-cvsserver: protect against NULL in crypt(3)","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-16T22:44:10Z","receivedAt":"2021-09-16T22:44:26Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, Sep 16, 2021 at 3:11 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n>\n> > -                if (crypt(descramble($password), $1) eq $1) {\n> > -                    $auth_ok = 1;\n> > +                my $hash = crypt(descramble($password), $1);\n> > +                if (defined $hash) {\n> > +                    $auth_ok = 1 if $hash eq $1;\n> >                  }\n>\n> It is not wrong per-se to separate the two checks into two separate\n> parts of the conditional, but because we check for definedness only\n> because comparison of it with $1 makes sense only when it is\n> defined, writing it either like this,\n>\n>                 if (defined $hash and $hash eq $1) {\n>                         $auth_ok = 1;\n>                 }\n>\n> or even like this,\n>\n>                 $auth_ok = (defined $hash and $hash eq $1);\n>\n> may be easier to read, no?\n\nyes, let's go with the earlier; I was trying to mimic the original\ncode, but agree on a second read that it looks confusing.\nassuming there are no more comments, would you want a reroll?\n\nCarlo\n"},{"id":"436199","messageId":"xmqqsfy350kc.fsf@gitster.g","threadId":"56501","inReplyTo":"CAPUEspjqD5zy8TLuFA96usU7FYi=0wF84y7NgOVFqegtxL9zbw@mail.gmail.com","subject":"Re: [PATCH 2/3] git-cvsserver: protect against NULL in crypt(3)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-17T03:43:31Z","receivedAt":"2021-09-17T03:43:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas <carenas@gmail.com> writes:\n\n>> It is not wrong per-se to separate the two checks into two separate\n>> parts of the conditional, but because we check for definedness only\n>> because comparison of it with $1 makes sense only when it is\n>> defined, writing it either like this,\n>>\n>>                 if (defined $hash and $hash eq $1) {\n>>                         $auth_ok = 1;\n>>                 }\n>>\n>> or even like this,\n>>\n>>                 $auth_ok = (defined $hash and $hash eq $1);\n>>\n>> may be easier to read, no?\n>\n> yes, let's go with the earlier; I was trying to mimic the original\n> code, but agree on a second read that it looks confusing.\n> assuming there are no more comments, would you want a reroll?\n\nIf there are no more comments, I certainly can tweak it myself, but\nI'd rather not have to keep an eye to see if the thread gets any\ncomment myself to begin with, as it will not scale as the process\nwhen there are multiple contributors and only one maintainer.\n\nSo, I'll make a local tweak myself and mark the topic as \"On hold\",\nto let you ping me after waiting for a while to see if the thread\ngets other comments that require a reroll.\n\nThanks.\n"}]}