Modify

Opened 6 years ago

Closed 4 years ago

Last modified 5 months ago

#4276 closed defect (fixed)

[patch] HtPasswdStore changes ownership of htpasswd file (due to bad file IO in Python fileinput used here)

Reported by: davclark@… Owned by: hasienda
Priority: high Component: AccountManagerPlugin
Severity: critical Keywords: htpasswd mangle file permission
Cc: felix.schwarz@… Trac Release: 0.10

Description

TSSIA! But for completeness:

First off, you could kind of call this a bug in python's fileinput.input( ... , inplace=True)... this actually moves the existing file to <filename>.bak and then writes a new file. This has the additional effect of deleting obviously named backup files that might be living there! At a minimum, I'd say you should supply a less likely argument to fileinput.input's backup argument!

That's really screwed me over once now! The plugin should definitely not be deleting such files!

When I manage an htpasswd file with AccountManagerPlugin, it changes group and user to whatever that process is running as. This is a big problem for shared hosting, where for example, an htpasswd file needs to be readable by the shared apache server. This server might be running under something like an "apache" group, but then this plugin changes the group to my personal user's default group.

Anyway, I've attached a diff against the htfile.py which uses more "old-school" file IO. I'm using it now without troubles. It could certainly be improved, and doesn't actually handle as many corner cases (e.g. duplicate entries) as the old version. But it's a step in the right direction, and I'm not personally worried about such corner cases!

Attachments (3)

htfile.py.diff (2.5 KB) - added by davclark@… 6 years ago.
htfile.py.2.diff (1.8 KB) - added by davclark 6 years ago.
A feature identical diff for htfile.py, apart from fixing this bug
20100930_reworked-for-discussion.patch (5.8 KB) - added by hasienda 4 years ago.
preview on suggested fix based on last patch

Download all attachments as: .zip

Change History (12)

Changed 6 years ago by davclark@…

comment:1 Changed 6 years ago by anonymous

I'm revisiting fixing the problems that my attached solution has and I've come up with a much simpler solution (conceptually). Specifically, if a file is opened in mode 'w', it is automatically truncated to empty, but otherwise retains unix permissions and ownership. So, I'd recommend the following model (and this is more or less what I'm using in my existing code):

---
password_lines = open('trac.htpasswd').readlines()

(changed, password_lines) = process(password_lines) # i.e. change a password or delete a line or add a line

if changed:

open('trac.htpasswd', 'w').writelines(password_lines)

---

You should of course break things out and trap file IO exceptions, but this allows you to do a quick proof of concept in ipython (if that's your fancy).

Thanks!
Dav

Changed 6 years ago by davclark

A feature identical diff for htfile.py, apart from fixing this bug

comment:2 Changed 6 years ago by anonymous

  • Cc felix.schwarz@… added

comment:3 Changed 4 years ago by hasienda

  • Keywords needinfo htpasswd mangle file permission added
  • Owner changed from pacopablo to hasienda
  • Priority changed from normal to high
  • Severity changed from normal to critical
  • Status changed from new to assigned
  • Summary changed from AccountManagerPlugin changes ownership of htpasswd file (due to implementation w/ fileinput) + extra "magical deletion" buglet to [patch] Plugin changes ownership of htpasswd file (due to implementation w/ fileinput) + extra "magical deletion" buglet

Effects confirmed by own tests even with trunk version. Sorry that we where unable to react on this issue for such a long time.

Since this is a DoS for shared use as mentioned in the report, I'll escalate this bug now.

BTW, thanks for the comments inside second patch file. I appreciate them a lot and will try to improve trunk code along these lines.

Q: While Trac is at 0.12 right now, how urgent would it be to back-port to 0.10 branch these days?

Changed 4 years ago by hasienda

preview on suggested fix based on last patch

comment:4 follow-up: Changed 4 years ago by hasienda

I've tested the proposed solution and made it the core of a changeset, that I've tested successfully so far and would like to commit really soon.

Dav, sorry for the long time of practically no support here. I'll try my best to catch up. Would you be so kind as to review my proposal as your time permits? Certainly anyone else's comments are welcome as well!

I've seen your comments and therefor got the impression, that you had some ideas on how to improve beyond the bare fix. I'm interested to hear them, since I'm not sure, if I got it already right. Would be great to do a good step forward with this code, since something like (good) credential file management is really basic for AccountManagerPlugin, right?

By own code studies I've got the impression, that we should use universal newline support, so line endings are always seen as '\n' by our code regardless of real formatting
(Macintosh convention '\r', Windows convention '\r\n'). Some code relies on the '\n' ending.
For write mode there is a 'w+' mode, that might be better for our purpose, but I'm not entirely clear about the differences. Do you know more than I could read in Python docs?

It seems a file could be left open for an indeterminated amount of time
under some circumstances, what wasn't reported to be an issue, but should be fixed as a precaution.

Finally read my comment regarding a suspected race-condition, please. Do you agree on the assumption? All I've read about file locking implementation is certainly non-trivial to achieve, if we demand some portable code. This could make up another ticket, but I raise the topic here as I see it somewhat related.

comment:5 Changed 4 years ago by hasienda

  • Summary changed from [patch] Plugin changes ownership of htpasswd file (due to implementation w/ fileinput) + extra "magical deletion" buglet to [patch] HtPasswdStore changes ownership of htpasswd file (due to bad file IO in Python fileinput used here)

#7401 has been closed as a duplicate of this ticket.

comment:6 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by davclark@…

Replying to hasienda:

I've seen your comments and therefor got the impression, that you had some ideas on how to improve beyond the bare fix. I'm interested to hear them, since I'm not sure, if I got it already right. Would be great to do a good step forward with this code, since something like (good) credential file management is really basic for AccountManagerPlugin, right?

Golly - it's been long enough that I don't remember the extra details of what I was thinking at the time. Certainly I don't have any code for you beyond what you already did (putting the 'open' calls in try blocks). I agree that using the with statement is nice going forwards, given the large amount of code and number of nested blocks. But not a big deal - the code as is seems pretty safe now in this regard.

By own code studies I've got the impression, that we should use universal newline support, so line endings are always seen as '\n' by our code regardless of real formatting
(Macintosh convention '\r', Windows convention '\r\n'). Some code relies on the '\n' ending.

Given that you're using universal newline support, it'd be marginally more robust to check f.newlines to find the newline convention used by the current file. This would be another step up from the old htfile code.

For write mode there is a 'w+' mode, that might be better for our purpose, but I'm not entirely clear about the differences. Do you know more than I could read in Python docs?

The python docs use poor language, I think. The modes are generally well-described in the C call, 'fopen.' But basically, the '+' means for reading and writing - almost never done anymore that I know of. Here's an example that illustrates the difference between 'w' and 'w+':

In [12]: f = open('test2.txt', 'w+')

In [13]: f.writelines(['This is a ...\n', 'Test!\n'])

In [14]: f.seek(0)

In [15]: f.readlines()
Out[15]: ['This is a ...\n', 'Test!\n']

-- vs. --

In [4]: f = open('test.txt', 'w')

In [5]: f.writelines(['This is a ...\n', 'Test!\n'])

In [6]: f.seek(0)

In [7]: f.readlines()


IOError Traceback (most recent call last)
/Users/dav/tmp/<ipython console> in <module>()
IOError: [Errno 9] Bad file descriptor

... i.e., the file is *only* open for writing. So, I think 'w' is marginally more appropriate in our case here.

It seems a file could be left open for an indeterminated amount of time
under some circumstances, what wasn't reported to be an issue, but should be fixed as a precaution.

Finally read my comment regarding a suspected race-condition, please. Do you agree on the assumption? All I've read about file locking implementation is certainly non-trivial to achieve, if we demand some portable code. This could make up another ticket, but I raise the topic here as I see it somewhat related.

Given bug #7401, it seems that at least one other person is using this in a shared hosting environment (dreamhost) where possibly subversion, Trac and who knows what else are sharing a htpasswd file. I don't think there is any way to prevent race conditions between systems.

In the end, this really is a hack (if you want concurrency, you should probably be using a database backend or at least an OS service). I've been running my patch 2 above for almost 2 years without incident now, and your patch is even more robust... so I wouldn't worry that much about it. These are going to be small deployments.

But certainly, you could implement some sort of lockfile that this code specifically would check. Could be a fun exploration, but I suspect we're only talking about a handful of headaches you'd be saving here.

comment:7 in reply to: ↑ 6 Changed 4 years ago by hasienda

  • Keywords needinfo removed

Replying to davclark@berkeley.edu:

![...]
Given that you're using universal newline support, it'd be marginally more robust to check f.newlines to find the newline convention used by the current file. This would be another step up from the old htfile code.

Indeed it seems better to check for a hint on actual newline format in f.newlines and use that instead of always '\n'. I didn't bother with this before, but it seems perfectly right. I'll try to implement this, as it could avoid some hassle for Trac with AccountManagerPlugin running on Windows and MacOS <= 9, right? But I'll not (yet) try file locking, as certainly this shouldn't be a big issue. I'll add a prominent warning to better go with any db backend for bigger application with concurrent credential use.

Your example in 'w' vs 'w+' was enlightening, and the whole response really did encourage me a lot to go on with this code. Thanks so much.

comment:8 Changed 4 years ago by davclark@…

Thank you!

comment:9 Changed 4 years ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [9272]) AccountManagerPlugin: Improve password file handling, closes #4276 and #4897.

htpasswd.bak files are not deleted anymore when updating htpasswd file
in the same directory and it's ownership is preserved as well, preventing
a DoS by inaccessible user file in shared use.
Furthermore we use universal newline support, if build-in, so line endings
are always seen as '\n' by our code regardless of real formatting
(Macintosh convention '\r', Windows convention '\r\n'). However the
actual end-of-line style is probed and preserved on file updates.
Finally the file is not left open for an indeterminated amount of time
after file access, what wasn't reported but deduced by own code studies
to be a potential issue and therefor fixed as a precaution.

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.