You are viewing a plain text version of this content. The canonical link for it is here.
Posted to bugs@apr.apache.org by bu...@apache.org on 2017/06/29 18:02:39 UTC
[Bug 61240] New: apr_file_transfer_contents change breaks
htpasswd(1)
https://bz.apache.org/bugzilla/show_bug.cgi?id=61240
Bug ID: 61240
Summary: apr_file_transfer_contents change breaks htpasswd(1)
Product: APR
Version: 1.6.2
Hardware: PC
OS: Linux
Status: NEW
Severity: major
Priority: P2
Component: APR
Assignee: bugs@apr.apache.org
Reporter: hlein-apbz@korelogic.com
Target Milestone: ---
Created attachment 35090
--> https://bz.apache.org/bugzilla/attachment.cgi?id=35090&action=edit
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org
[Bug 61240] apr_file_transfer_contents change breaks htpasswd(1)
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61240
Nick Kew <ni...@webthing.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |NEEDINFO
--- Comment #2 from Nick Kew <ni...@webthing.com> ---
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?
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org
[Bug 61240] apr_file_transfer_contents change breaks htpasswd(1)
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61240
hlein-apbz@korelogic.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEEDINFO |NEW
--- Comment #3 from hlein-apbz@korelogic.com ---
(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!
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org
[Bug 61240] apr_file_transfer_contents change breaks htpasswd(1)
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61240
Ruediger Pluem <rp...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |NEEDINFO
--- Comment #4 from Ruediger Pluem <rp...@apache.org> ---
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);
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org
[Bug 61240] apr_file_transfer_contents change breaks htpasswd(1)
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61240
--- Comment #1 from hlein-apbz@korelogic.com ---
s/1\.62/1.6.2/g
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org
[Bug 61240] apr_file_transfer_contents change breaks htpasswd(1)
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61240
Ruediger Pluem <rp...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Component|APR |support
Product|APR |Apache httpd-2
Status|NEW |RESOLVED
Keywords| |FixedInTrunk,
| |PatchAvailable
Version|1.6.2 |2.5-HEAD
Resolution|--- |FIXED
Assignee|bugs@apr.apache.org |bugs@httpd.apache.org
--- Comment #6 from Ruediger Pluem <rp...@apache.org> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org
[Bug 61240] apr_file_transfer_contents change breaks htpasswd(1)
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=61240
Hank Leininger <hl...@korelogic.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEEDINFO |NEW
--- Comment #5 from Hank Leininger <hl...@korelogic.com> ---
(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!
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: bugs-unsubscribe@apr.apache.org
For additional commands, e-mail: bugs-help@apr.apache.org