[pmwiki-devel] The (In)Security of IncludeUpload

[Kathryn Andersen]

On Wed, May 02, 2007 at 08:01:54AM -0500, Patrick R. Michaud wrote:
> On Wed, May 02, 2007 at 12:33:02PM +1000, Kathryn Andersen wrote:
> > There are a few issues I'm aware of:
> > 
> > 1. the command to do text-to-html conversion is a `command`, which
> > isn't secure, since someone could pass in arguments which would turn
> > it into `command arg;rm -rf /*` or the like.  With Perl, there are
> > things like taint.  I don't know what the equivalent is in PHP.
> 
> PHP ain't got no taint.  :-)  Besides, even if it did, the 
> variables you're wanting to use here ($txt2html_args) would 
> be tainted, because they're coming from the "outside world".
> 
> Out of curiosity, what `command` are you typically using to
> convert the text to html?  

The command is user-defined.  This is because I'm aware of at least two
good text-to-html converters (txt2html and asciidoc) and I wanted to
allow the wiki admin to choose whatever converter they wanted.
The command is defined in $IncludeUploadTxtToHtmlCmd
(though I'm thinking of changing this so that more than one "conversion"
command can be defined, so that people can do custom conversions on
things other than plain text files).  By default, the command is empty,
because people may not have a converter installed, and in that case it
just puts <pre> around the text.

So I have no control over what the actual command is, but since the
command is defined by the wiki administrator, then I think that's a
reasonable risk.
 
> One possibility might be to explicitly list the $txt2html_args
> that you're willing to accept, rather than take them directly
> from the directive argument.  (If you can provide a couple of
> examples, I can illustrate a way to do this.)

Since there can be completely different converters, I'm afraid this
approach won't work.  But I don't want to prevent editors passing
arguments in altogether, because I find it useful myself.

For example, for a text file which I know is a poem, I want to be able
to re-define the length of "short lines" do be quite long, so that the
converter (I use txt2html) will honour the line-breaks, but I only want
this to be done for poems, not other text files.
 
> > 2. Files stored in pmwiki/uploads are not checked to see whether
> > the user has read permission on them.  [...]
> > 3. Files stored under $DOCUMENT_ROOT do not check Apache permissions,
> > they just go straight to the filesystem, which means that if the
> > file is readable by Apache, then it's readable by anyone.
> > I don't know how to check for this.
> 
> One answer might be to read the file's contents via url instead
> of directly off the filesystem (i.e., using a url-based fopen).

Hmmm.  Would this break for systems where the system admin has disabled
that kind of fopen?
 
> > I'm not sure how one
> > actually defines "read permission" for uploaded files, since
> > PmWiki puts permissions on wiki pages, and it isn't clear how one
> > would determine to which page a particular uploaded file "belongs",
> > in order to see if the user is allowed to read the uploaded file.
> > Presumably PmWiki does some sort of check when secure uploads are turned
> > on, but I don't know how or where that's done.
> 
> When attachments are being protected ($EnableDirectDownload=0;), 
> PmWiki uses a page's read permissions to determine whether a 
> visitor has permission to view any of the attachments for that page.
> There's not really a concept of a separate set of permissions
> for individual attachment files.

So that means then that there's no point in trying to check the
permission of an *included* upload file, since the markup won't be
rendered unless the visitor has read permission for that page anyway.
Is that correct?

Thanks for your help!

Kathryn Andersen
-- 
 _--_|\     | Kathryn Andersen	<http://www.katspace.com>
/      \    | 
\_.--.*/    | GenFicCrit mailing list <http://www.katspace.com/gen_fic_crit/>
      v     | 
------------| Melbourne -> Victoria -> Australia -> Southern Hemisphere
Maranatha!  |	-> Earth -> Sol -> Milky Way Galaxy -> Universe