Edgewall Software
Modify

Opened 7 years ago

Closed 7 years ago

#12744 closed defect (fixed)

Improve handling of htpasswd file

Reported by: anonymous Owned by: Ryan J Ollos
Priority: normal Milestone: 1.2.2
Component: web frontend/tracd Version:
Severity: normal Keywords: htpasswd authentication
Cc: Branch:
Release Notes:

Extra entries and comments in htpasswd and htdigest files are ignored.

API Changes:
Internal Changes:

Description

As explained in this pull request (https://github.com/edgewall/trac/pull/7) the handling of htpasswd files for HTTP Basic Auth in tracd is not very robust. In particular, it cannot handle basic formatting that other tools (notably apache and nginx) can handle — and it fails in ways that could allow logins that are not intended to be valid. It does not handle comment lines, so that logins that are intended to be commented out are just logins with a # prefixed. Also, if more than just the login name and password are provided, the entire line is printed to the log, exposing passwords. This breaks compatibility with other tools like dokuwiki, which users more fields.

The fix is fairly simple: remove everything after a #, and ignore all but the first two components of any line that remains.

Attachments (0)

Change History (6)

in reply to:  description comment:1 by Ryan J Ollos, 7 years ago

Replying to anonymous:

The fix is fairly simple: remove everything after a #, and ignore all but the first two components of any line that remains.

Is the htpasswd format formally documented anywhere? I assume this should apply to htdigest files as well.

We should consider just making passlib a Trac dependency and using passlib.apache.Htpasswd. If not immediately, then at least when we integrate some pieces of AccountManagerPlugin.

comment:2 by anonymous, 7 years ago

TL;DR: Passlib is also broken. Apache, nginx, and dokuwiki each actively support additional colon-separated fields, and so should you.


Judging from this comment, it looks like passlib also goes by the output from the htpasswd program, rather than acceptable input to the httpd function that reads the files. In particular, the parsing function raises an exception if there are not exactly 2 fields that result from a split by colons. But this behavior, like trac's, is inconsistent with apache, nginx, and dokuwiki.

What little documentation there is for apache appears to be here. The actual code in apache that does the reading of each line is here. The key lines where the username and password are extracted are

w = ap_getword(r->pool, &rpw, ':');

and

file_password = ap_getword(r->pool, &rpw, ':');

Here, &rpw is a const char**. This reference says that inside ap_getword, the "char** is updated after each call so that it points to the place where the previous call left off." So that second call to get file_password is explicitly allowing the line to have another colon after the password itself — though if that colon is not found before the end of line, that is also acceptable. (Note that apache also skips blank lines or lines beginning with #, as seen just before those calls.)

Similarly, the nginx function looks for the end of the password by searching for a line ending (LF or CR) or a colon. (Again, nginx skips blank lines or lines beginning with #, as seen shortly before the line I linked.)

I won't bother looking through dokuwiki's source; instead I'll just point out that the documentation here explicitly gives the list of colon-separated fields it uses.

comment:3 by anonymous, 7 years ago

I've noted this problem as a passlib issue.

comment:4 by Ryan J Ollos, 7 years ago

Keywords: htpasswd authentication added
Milestone: 1.2.2
Owner: set to Ryan J Ollos
Status: newassigned

I don't see a problem with ignoring everything including and after the second :. We'll need to add a test for the change, and also look into making a similar change for HtDigest.

comment:5 by Ryan J Ollos, 7 years ago

Proposed changes in [d3e4955cf/rjollos.git].

comment:6 by Ryan J Ollos, 7 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to 1.2-stable in r15922, merged to trunk in r15923.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.