expansion of %(uri)s in when_done parameter using Python's % operator is ill-advised #1860

New Issue

tahoe-lafs · 2012-11-15T03:31:28Z

davidsarah commented

2012-11-15 03:31:28 +00:00

At source:git/src/allmydata/web/unlinked.py@3d771132#L52 we see:

            if "%(uri)s" in redir_to:
                redir_to = redir_to % {"uri": urllib.quote(upload_results.get_uri())
                                         }

This is intended to expand %(uri)s in the when_done parameter of an unlinked upload, to the URI of the new uploaded file.

Python isn't straightforwardly vulnerable to C printf-style format attacks. However, the % operator is still not designed to take untrusted input on the left, and it's a bad idea to use it that way. If nothing else, it is completely undocumentable except by reference to the Python format string documentation. Also, any % characters, i.e. URL escapes, in the when_done URL will have to be doubled (encoded as %25%25 in the original URL) so that they are not interpreted as format characters.

It isn't clear that %(uri)s should continue to be supported, but if it is, then it shouldn't be implemented this way.

At source:git/src/allmydata/web/unlinked.py@3d771132#L52 we see: ``` if "%(uri)s" in redir_to: redir_to = redir_to % {"uri": urllib.quote(upload_results.get_uri()) } ``` This is intended to expand `%(uri)s` in the `when_done` parameter of an unlinked upload, to the URI of the new uploaded file. Python isn't straightforwardly vulnerable to C `printf`-style format attacks. However, the `%` operator is still not designed to take untrusted input on the left, and it's a bad idea to use it that way. If nothing else, it is completely undocumentable except by reference to the [Python format string documentation](http://docs.python.org/2/library/stdtypes.html#string-formatting-operations). Also, any `%` characters, i.e. URL escapes, in the `when_done` URL will have to be doubled (encoded as `%25%25` in the original URL) so that they are not interpreted as format characters. It isn't clear that `%(uri)s` should continue to be supported, but if it is, then it shouldn't be implemented this way.

tahoe-lafs added the

labels 2012-11-15 03:31:28 +00:00

tahoe-lafs added this to the undecided milestone 2012-11-15 03:31:28 +00:00

David-Sarah Hopwood <david-sarah@jacaranda.org> commented

2012-11-15 04:17:50 +00:00

In changeset:e097cf96b292f948:

web/unlinked.py: don't use % operator to expand %(uri)s. fixes #1860.

Signed-off-by: David-Sarah Hopwood <david-sarah@jacaranda.org>

In changeset:e097cf96b292f948: ``` web/unlinked.py: don't use % operator to expand %(uri)s. fixes #1860. Signed-off-by: David-Sarah Hopwood <david-sarah@jacaranda.org> ```

tahoe-lafs added the

fixed

label 2012-11-15 04:17:50 +00:00

tahoe-lafs closed this issue

2012-11-15 04:17:50 +00:00

tahoe-lafs changed title from ~~expansion of %(uri)s in when_done parameter is ill-advised~~ to expansion of %(uri)s in when_done parameter using Python's % operator is ill-advised

2012-11-15 04:20:33 +00:00

tahoe-lafs modified the milestone from undecided to 1.10.0

2012-11-15 04:21:33 +00:00

Sign in to join this conversation.