Created attachment 35090 [details] Revert the change in behavior of apr_file_transfer_contents which breaks htpasswd files. A recent apr commit changed apr_file_transfer_contents to always set the destination file to match the permissions of the source file, even if the destination file already exists with different permissions. This breaks htpasswd(1) in common configurations. The one-line change was committed here: https://svn.apache.org/viewvc?view=revision&revision=1791029 ...after being briefly discussed on the apr-dev mailing list, starting here: https://marc.info/?l=apr-dev&m=149088210914254&w=2 This change had an unintended consequence that causes htpasswd(1) to break htpasswd files' permissions. The htpasswd file needs to be readable by Apache's non-root user, so it is common for it to be root:apache mode 640 or so. When using htpasswd(1) to update it, a tempfile is created mode 600, and then apr_file_copy (which calls apr_file_transfer_contents) is used to copy tempfile contents to the real htpasswd file. Since the above change landed in apr 1.62, the permissions of the real htpasswd file are clobbered, explicitly chmod'ed to 600 to match that of the tempfile. All access to content protected by htpasswd will now fail, because Apache cannot read the file. Probably this means htpasswd(1) is using apr_file_copy incorrectly, but its use has not changed for over a decade. It could easily be that other tools depend on the longstanding behavior. Recommend this change be reverted until/unless all users of apr_file_transfer_contents can be checked for the ramifications.
s/1\.62/1.6.2/g
The change sets file perms to those of the source file if and only if the to_perms argument tells it to. So if there's a bug, it's in the arguments with which apr_file_transfer_contents is called. I just tried running htpasswd under gdb with a breakpoint on apr_file_transfer_contents, but it never reached the breakpoint. Can you supply a test case that demonstrates your problem?
(In reply to Nick Kew from comment #2) > The change sets file perms to those of the source file if and only if the > to_perms argument tells it to. So if there's a bug, it's in the arguments > with which apr_file_transfer_contents is called. Yeah, probably htpasswd(1) is calling apr_file_copy incorrectly, but it has been doing so forever: apache-git/httpd $ git blame support/htpasswd.c | egrep apr_file_copy 9229359a77b (Thom May 2004-03-13 22:18:19 +0000 506) if (apr_file_copy(dirname, pwfilename, APR_FILE_SOURCE_PERMS, pool) != It does seem like APR_FILE_SOURCE_PERMS is the wrong value to pass here, and it has worked all along only because of the actual behavior of apr_file_copy. Fixing apr_file_copy broke this longstanding wrong use - and perhaps other callers of it also have baked-in assumptions of its wrong behavior. I only see two callers of apr_file_copy in httpd.git, htpasswd.c and htdigest.c; the latter is very similar especially in its file-handling code. I have no idea what other Apache projects link to libapr and might also (mis)use it and whose bugs this change will now trigger. > I just tried running htpasswd under gdb with a breakpoint on > apr_file_transfer_contents, but it never reached the breakpoint. Can you > supply a test case that demonstrates your problem? Hm, strange. It is 100% reproducible here. Using strace since I am garbage with gdb, and batch mode (-b) to simplify the example (has no impact on the bad behavior by htpasswd): # ldd `which htpasswd` | egrep apr- libapr-1.so.0 => /usr/lib64/libapr-1.so.0 (0x000003072eec4000) # ls -l /usr/lib64/libapr-1.so.0 lrwxrwxrwx 1 root root 17 Jun 29 12:41 /usr/lib64/libapr-1.so.0 -> libapr-1.so.0.6.2 # umask 0022 # ls -l test_htpasswd -rw-r----- 1 root apache 47 Jun 29 23:06 test_htpasswd # strace htpasswd -b -m test_htpasswd testuser testpass 2>&1 | egrep -2 'chmod|O_CREAT' read(3, "\26\341$D", 4) = 4 close(3) = 0 open("/tmp/apr-tmp.xwEPYS", O_RDWR|O_CREAT|O_EXCL, 0600) = 3 fcntl(3, F_GETFD) = 0 fcntl(3, F_SETFD, FD_CLOEXEC) = 0 -- close(3) = 0 unlink("/tmp/apr-tmp.xwEPYS") = 0 open("/tmp/htpasswd.tmp.4fZ6Oj", O_RDWR|O_CREAT|O_EXCL, 0600) = 3 fcntl(3, F_GETFD) = 0 fcntl(3, F_SETFD, FD_CLOEXEC) = 0 -- open("/tmp/htpasswd.tmp.4fZ6Oj", O_RDONLY|O_CLOEXEC) = 4 fstat(4, {st_mode=S_IFREG|0600, st_size=47, ...}) = 0 chmod("test_htpasswd", 0600) = 0 open("test_htpasswd", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0600) = 5 read(4, "testuser:$apr1$qXiIfnVS$aGBhoQeU"..., 8192) = 47 write(5, "testuser:$apr1$qXiIfnVS$aGBhoQeU"..., 47) = 47 # ls -l test_htpasswd -rw------- 1 root apache 47 Jun 29 23:08 test_htpasswd First a tempfile is created mode 0600 (good, sane). Then a second, also mode 0600. Then that second is fstat'ed, and the existing htpasswd file is chmod'ed with the specific perms from that second tempfile. Tested using htpasswd(1) from both Apache 2.2.mumble and 2.4.26; the relevant part of support/htpasswd.c is unchanged, as shown above. Please let me know if this is not enough to reproduce. Thanks!
IMHO APR code is now correct and this is a bug in htpasswd. Does the following patch to htpasswd fix the issue for you? Index: htpasswd.c =================================================================== --- htpasswd.c (revision 1800082) +++ htpasswd.c (working copy) @@ -498,7 +498,7 @@ /* The temporary file has all the data, just copy it to the new location. */ - if (apr_file_copy(dirname, pwfilename, APR_FILE_SOURCE_PERMS, pool) != + if (apr_file_copy(dirname, pwfilename, APR_OS_DEFAULT, pool) != APR_SUCCESS) { apr_file_printf(errfile, "%s: unable to update file %s" NL, argv[0], pwfilename);
(In reply to Ruediger Pluem from comment #4) > IMHO APR code is now correct and this is a bug in htpasswd. Yes, I agree that htpasswd.c is using apr_file_copy incorrectly. And has been doing so forever. > Does the > following patch to htpasswd fix the issue for you? > > Index: htpasswd.c > =================================================================== > --- htpasswd.c (revision 1800082) > +++ htpasswd.c (working copy) > @@ -498,7 +498,7 @@ > > /* The temporary file has all the data, just copy it to the new > location. > */ > - if (apr_file_copy(dirname, pwfilename, APR_FILE_SOURCE_PERMS, pool) != > + if (apr_file_copy(dirname, pwfilename, APR_OS_DEFAULT, pool) != > APR_SUCCESS) { > apr_file_printf(errfile, "%s: unable to update file %s" NL, > argv[0], pwfilename); Indeed, that does seem to do the right thing. So every caller of apr_file_copy or apr_file_transfer_contents ought to be reviewed to make sure they are not using APR_FILE_SOURCE_PERMS flag (or equivalent APR_FPROT_FILE_SOURCE_PERMS) when they do not mean it. But in the meantime shouldn't the fix to apr_file_transfer_contents be reverted, and instead announced as upcoming so projects that use libapr can check? Unless you can easily tell who all users of apr_file_copy / apr_file_transfer_contents are, and can confirm that none of them will have this problem leading to surprise failures. That sounds a) not easy and b) not the job of apr-developers? Thanks!
(In reply to Hank Leininger from comment #5) > (In reply to Ruediger Pluem from comment #4) > > IMHO APR code is now correct and this is a bug in htpasswd. > > Yes, I agree that htpasswd.c is using apr_file_copy incorrectly. And has > been doing so forever. > > > Does the > > following patch to htpasswd fix the issue for you? > > > > Index: htpasswd.c > > =================================================================== > > --- htpasswd.c (revision 1800082) > > +++ htpasswd.c (working copy) > > @@ -498,7 +498,7 @@ > > > > /* The temporary file has all the data, just copy it to the new > > location. > > */ > > - if (apr_file_copy(dirname, pwfilename, APR_FILE_SOURCE_PERMS, pool) != > > + if (apr_file_copy(dirname, pwfilename, APR_OS_DEFAULT, pool) != > > APR_SUCCESS) { > > apr_file_printf(errfile, "%s: unable to update file %s" NL, > > argv[0], pwfilename); > > Indeed, that does seem to do the right thing. Comitted to httpd-trunk as r1800594. > > So every caller of apr_file_copy or apr_file_transfer_contents ought to be > reviewed to make sure they are not using APR_FILE_SOURCE_PERMS flag (or > equivalent APR_FPROT_FILE_SOURCE_PERMS) when they do not mean it. > > But in the meantime shouldn't the fix to apr_file_transfer_contents be > reverted, and instead announced as upcoming so projects that use libapr can > check? Unless you can easily tell who all users of apr_file_copy / > apr_file_transfer_contents are, and can confirm that none of them will have > this problem leading to surprise failures. That sounds a) not easy and b) > not the job of apr-developers? No, we do not remove a fix for a bug in APR. The documentation here was clear and it is a clear bug of the caller if it uses APR_FILE_SOURCE_PERMS / APR_FPROT_FILE_SOURCE_PERMS and if does not want that to happen.
Backported to 2.4.x as r1800775. Will be part of 2.4.27.