These instances of code similar to OVE-20170111-0001 are not believed
to be exploitable, because defined(), length(), setpassword(),
userinfo_set() and the binary "." operator all have prototypes that
force the relevant argument to be evaluated in scalar context. However,
using a safer idiom makes mistakes less likely.
(cherry picked from commit 69230a2220f673c66b5ab875bfc759b32a241c0d)
CGI::FormBuilder->field has behaviour similar to the CGI.pm misfeature
we avoided in f4ec7b0. Force it into scalar context where it is used
in an argument list.
This prevents two (relatively minor) commit metadata forgery
vulnerabilities:
* In the comments plugin, an attacker who was able to post a comment
could give it a user-specified author and author-URL even if the wiki
configuration did not allow for that, by crafting multiple values
to other fields.
* In the editpage plugin, an attacker who was able to edit a page
could potentially forge commit authorship by crafting multiple values
for the rcsinfo field.
The remaining plugins changed in this commit appear to have been
protected by use of explicit scalar prototypes for the called functions,
but have been changed anyway to make them more obviously correct.
In particular, checkpassword() in passwordauth has a known prototype,
so an attacker cannot trick it into treating multiple values of the
name field as being the username, password and field to check for.
OVE-20161226-0001
CGI->param has the misfeature that it is context-sensitive, and in
particular can expand to more than one scalar in function calls.
This led to a security vulnerability in Bugzilla, and recent versions
of CGI.pm will warn when it is used in this way.
In the situations where we do want to cope with more than one parameter
of the same name, CGI->param_fetch (which always returns an
array-reference) makes the intention clearer.
[commit message added by smcv]
When CGI->param is called in list context, such as in function
parameters, it expands to all the potentially multiple values
of the parameter: for instance, if we parse query string a=b&a=c&d=e
and call func($cgi->param('a')), that's equivalent to func('b', 'c').
Most of the functions we're calling do not expect that.
I do not believe this is an exploitable security vulnerability in
ikiwiki, but it was exploitable in Bugzilla.
As noted in the Try::Tiny man page, eval/$@ can be quite awkward in
corner cases, because $@ has the same properties and problems as C's
errno. While writing a regression test for definetemplate
in which it couldn't find an appropriate template, I received
<span class="error">Error: failed to process template
<span class="createlink">deftmpl</span> </span>
instead of the intended
<span class="error">Error: failed to process template
<span class="createlink">deftmpl</span> template deftmpl not
found</span>
which turned out to be because the "catch"-analogous block called
gettext before it used $@, and gettext can call define_gettext,
which uses eval.
This commit alters all current "catch"-like blocks that use $@, except
those that just do trivial things with $@ (string interpolation, string
concatenation) and call a function (die, error, print, etc.)
Previously, prune("wiki/srcdir/sandbox/test.mdwn") could delete srcdir
or even wiki, if they happened to be empty. This is rarely what you
want: there's usually some base directory (destdir, srcdir, transientdir
or another subdirectory of wikistatedir) beyond which you do not want to
delete.
Foo->Bar->can("method") works just as well, even if Foo::Bar is not
loaded. Using UNIVERSAL::can is deprecated.
But, I was unable to easily eliminate conditional.pm's use of UNIVERSAL::can
Firefox sent an accept header for application/xml, not application/json,
and also weakened the priority to 0.8. So that stuff is not to be trusted;
instead I found a better way: When an ajax upload is *not* being made,
the Upload Attachment button will be used, so enable ajax if an upload
is being made without that button having been used.
Also, testing with firefox revealed it refused to process a response that
was type application/json, and checking the demo page for the jquery file
upload plugin, it actually returns the json with type text/html. Ugh.
Followed suite.
Now tested with: chromium, chromium (w/o js), firefox, firefox (w/o js),
and w3m.
Needed for attachment to return json when requested.
I think some browsers send Accept: * , so I made sure to check that json
was explicitly listed as to be accepted, as well as having a high
priority.
Left out confirmation of removal for held attachments because
a) they're not in the wiki yet, so confirmation is a bit unnecessary
b) it would be hard
c) eases later integration of jquery file upload interface
Also changed where attachments of index are held (to match where they're
stored in the srcdir).
Note that the attachment formbuilder hook was made to run last, so that
the list of attachments is not generated before removal, in the fast path
w/o confirm.
Note that it's possible for an attachment in the holding area to be older
than an attachemnt in the wiki with the same name. I intentionally
show the one in the holding area in this (unlikely) case, since saving the
page will overwrite the wiki's file with the held attachment. It does not
seem worth the bother of doing something more intelligent, since in this
case two people have basically conflicted with one-another.. and both
attachment contents will be stored in revision control in case it needs to
be sorted out.
I had to remove the hyperlink for attachments in the holding area, since
they're not yet live on the web. This could be annoying/confusing. Added
a moseover notice instead.
This makes uploading a lot of attachments somewhat faster, because
the user does not need to wait for a long website refresh after each
upload. Still probably somewhat slow, since ikiwiki has to run for each
upload.
More importantly, this opens the door for integration of things like
the jquery file upload interface, which allow drag-n-drop and multiple
file uploads to be queued and then ran.
It uses rcs_commit_staged, which leaves out tla and mercurual which lack
that, but since rename, remove, autoindex, etc also use that, I think it's
fine for attachments to also depend on it.
The attachment list is currently broken; it does not look in the holding
area yet, and its links to the attached files won't work since they're not
yet in the wiki. previewing is also currently broken.
Work sponsored by TOVA.
Using named parameters for these is overdue. Passing the session in a
parameter instead of passing username and IP separately will later allow
storing other session info, like username or part of the email.
Note that these functions are not part of the exported API,
and the prototype change will catch (most) skew, so I am not changing
API versions. Any third-party plugins that call them will need updated
though.
Everywhere that REMOTE_ADDR was used, a session object is available, so
instead use its remote_addr method.
In IkiWiki::Receive, stop setting a dummy REMOTE_ADDR.
Note that it's possible for a session cookie to be obtained using one IP
address, and then used from another IP. In this case, the first IP will now
be used. I think that should be ok.
Many calls to file_prune were incorrectly calling it with 2 parameters.
In cases where the filename being checked is relative to the srcdir,
that is not needed.
Made absolute filenames be pruned. (This won't work for the 2 parameter call
style.)
A new ikiwiki-transition moveprefs subcommand can pull the old data out of
the userdb and inject it into the setup file.
Note that it leaves the old values behind in the userdb too. I did this
because I didn't want to lose data if it fails writing the setup file for
some reason, and the old data in the userdb will only use a small amount of
space. Running the command multiple times will mostly not change anything.
Simon McVittie
Amitai Schlair
Joey Hess
Joey Hess
Joey Hess