ikiwiki's web interface does not currently have UI for removing
multiple pages simultaneously, but the remove plugin is robust
against doing so. Use a clearer idiom to make that obvious.
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)
Calling CGI::FormBuilder::field with a name argument in list context
returns zero or more user-specified values of the named field, even
if that field was not declared as supporting multiple values.
Passing the result of field as a function parameter counts as list
context. This is the same bad behaviour that is now discouraged
for CGI::param.
In this case we pass the multiple values to CGI::Session::param.
That accessor has six possible calling conventions, of which four are
documented. If an attacker passes (2*n + 1) values for the 'name'
field, for example name=a&name=b&name=c, we end up in one of the
undocumented calling conventions for param:
# equivalent to: (name => 'a', b => 'c')
$session->param('name', 'a', 'b', 'c')
and the 'b' session parameter is unexpectedly set to an
attacker-specified value.
In particular, if an attacker "bob" specifies
name=bob&name=name&name=alice, then authentication is carried out
for "bob" but the CGI::Session ends up containing {name => 'alice'},
an authentication bypass vulnerability.
This vulnerability is tracked as OVE-20170111-0001.
(cherry picked from commit e909eb93f4530a175d622360a8433e833ecf0254)
git_sha1 already puts "--" before its arguments, so
git_sha1_file($dir, 'doc/index.mdwn')
would have incorrectly invoked
git rev-list --max-count=1 HEAD -- -- doc/index.mdwn
If there is no file in the wiki named "--", that's harmless, because
it merely names the latest revision in which either "--" or
"doc/index.mdwn" changed. However, it could return incorrect results
if there is somehow a file named "--".
If we throw an exception (usually from run_or_die), in_git_dir won't
unshift the current directory from the stack. That's usually fine,
but in rcs_preprevert we catch exceptions and do some cleanup before
returning, for which we need the git directory to be the root and
not the temporary working tree.
Some of these might be relatively expensive to dereference or result
in messages being logged, and there's no reason why a search engine
should need to index them. (In particular, we'd probably prefer search
engines to index the rendered page, not its source code.)
We exclude .git/hooks from symlinking into the temporary working tree,
which avoids the commit hook being run for the temporary branch anyway.
This avoids the wiki not being updated if an orthogonal change is
received in process A, while process B prepares a revert that is
subsequently cancelled.
Otherwise, we have a time-of-check/time-of-use vulnerability:
rcs_preprevert previously looked at what changed in the commit we are
reverting, not at what would result from reverting it now. In
particular, if some files were renamed since the commit we are
reverting, a revert of changes that were within the designated
subdirectory and allowed by check_canchange() might now affect
files that are outside the designated subdirectory or disallowed
by check_canchange().
It is not sufficient to disable rename detection, since git older
than 2.8.0rc0 (in particular the version in Debian stable) silently
accepts and ignores the relevant options.
OVE-20161226-0002
This doesn't work prior to git 2.8: `git revert` silently ignores the
option and succeeds. We will have to fix CVE-2016-10026 some other way.
This reverts commit 9cada49ed6.
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
Otherwise, we have an authorization bypass vulnerability: rcs_preprevert
looks at what changed in the commit we are reverting, not at what would
result from reverting it now. In particular, if some files were renamed
since the commit we are reverting, a revert of changes that were within
the designated subdirectory and allowed by check_canchange() might now
affect files that are outside the designated subdirectory or disallowed
by check_canchange().
Signed-off-by: Simon McVittie <smcv@debian.org>
Otherwise, if third-party plugins extend newenviron by more than
3 entries, we could overflow the array. It seems unlikely that any
third-party plugin manipulates newenviron in practice, so this
is mostly theoretical. Just in case, I have deliberately avoided
using "i" as the variable name, so that any third-party plugin
that was manipulating newenviron directly will now result in the
wrapper failing to compile.
I have not assumed that realloc(NULL, ...) works as an equivalent of
malloc(...), in case there are still operating systems where that
doesn't work.
SVG scaling by img directives has subtly changed; where before size=wxh
would preserve aspect ratio, this cannot be done when passing them through
and so specifying both a width and height can change the SVG's aspect
ratio.
(This patch looks significantly more complex than it was, because a large
block of code had to be indented.)
[smcv: drop trailing whitespace, fix some spelling]
This mitigates CVE-2016-3714. Wiki administrators who know that they
have prevented arbitrary code execution via other formats can re-enable
the other formats if desired.
A site administrator might unwisely set allowed_attachments to
something like '*.jpg or *.png'; if they do, an attacker could attach,
for example, a SVG file named attachment.jpg.
This mitigates CVE-2016-3714.
The instance in cgierror() is a potential cross-site scripting attack,
because an attacker could conceivably cause some module to raise an
exception that includes attacker-supplied HTML in its message, for
example via a crafted filename. (OVE-20160505-0012)
The instances in preprocess() is just correctness. It is not a
cross-site scripting attack, because an attacker could equally well
write the desired HTML themselves; the sanitize hook is what
protects us from cross-site scripting here.
If the relative link from the (page generating the) RSS to the target
would start with "./" or "../", just concatenating it with the URL to
the directory containing the RSS is not sufficient. Go via
URI::new_abs to fix this.
Now I'm going to get bug reports about wanting the URLs to be
protocol-relative, but we can't win there as long as we generate RSS,
because RSS doesn't have well-defined semantics for relative URLs
(and the W3C's validator complains about them). If absolute URLs are
a problem for you, please use Atom feeds.
Simon McVittie
Amitai Schleier
intrigeri
Joey Hess
Florian Wagner