Arch Linux

Please read this before reporting a bug:
https://wiki.archlinux.org/index.php/Reporting_Bug_Guidelines

Do NOT report bugs when a package is just outdated, or it is in Unsupported. Use the 'flag out of date' link on the package page, or the Mailing List.

REPEAT: Do NOT report bugs for outdated packages!
Tasklist

FS#57526 - [patch] [Security] multiple issues (CVE-2018-6952 CVE-2018-6951)

Attached to Project: Arch Linux
Opened by Morten Linderud (Foxboron) - Thursday, 15 February 2018, 23:37 GMT
Last edited by Sébastien Luttringer (seblu) - Monday, 12 November 2018, 21:17 GMT
Task Type Bug Report
Category Security
Status Assigned   Reopened
Assigned To Sébastien Luttringer (seblu)
Levente Polyak (anthraxx)
Architecture All
Severity Medium
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

Summary
=======

The package patch is vulnerable to multiple issues including arbitrary code execution and denial of service via CVE-2018-6952 and CVE-2018-6951.

Guidance
========

Cherry pick the given patches

References
==========

https://security.archlinux.org/AVG-619
https://savannah.gnu.org/bugs/index.php?53133
https://git.savannah.gnu.org/cgit/patch.git/commit/?id=f290f48a621867084884bfff87f8093c15195e6a
https://savannah.gnu.org/bugs/index.php?53132
This task depends upon

Comment by Sébastien Luttringer (seblu) - Sunday, 18 February 2018, 22:31 GMT
I didn't find a patch for CVE-2018-6952. Will wait for both patch before push.
Comment by Sébastien Luttringer (seblu) - Thursday, 08 March 2018, 11:48 GMT
I'm correct if I says there is still no fix taken by upstream for CVE-2018-6952?
Comment by Morten Linderud (Foxboron) - Friday, 18 May 2018, 20:44 GMT
Sorry for the delay.
Seems like CVE-2018-6952 hasn't had a fix pushed so far. However CVE-2018-1000156 is now a thing and has multiple related commits in the tree. I think all commits from 04-06-2018 and onwards are related, but unsure.
Comment by Carlos Silva (r3pek) - Tuesday, 25 September 2018, 10:30 GMT Comment by Eli Schwartz (eschwartz) - Friday, 09 November 2018, 20:32 GMT
Unfortunately, some patches were never applied as the PKGBUILD makes use of some mysterious auto-patch thing that uses heuristics in order to not find and apply the patches.

See https://bbs.archlinux.org/viewtopic.php?id=241814
Comment by Sébastien Luttringer (seblu) - Friday, 09 November 2018, 23:06 GMT
It takes files ending by .patch. What is mysterious heuristics?
Comment by harrigan (harrigan) - Friday, 09 November 2018, 23:24 GMT
The for loop in prepare() looks for entries in the source array that end in .patch but the 3 savannah entries don't. So, for example, prepare() skips over http://git.savannah.gnu.org/cgit/patch.git/patch/?id=123eaff0d5d1aebe128295959435b9ca5909c26d because "$filename" =~ \.patch$ fails.
Comment by Sébastien Luttringer (seblu) - Friday, 09 November 2018, 23:26 GMT
Yes, so when files not end with .patch, we need to do call patch the old fashion way. Blame the packager, not the tool...

Btw, the patches doesn't apply as is.
Comment by loqs (loqs) - Friday, 09 November 2018, 23:59 GMT
Adapted patches that apply after the two patches sourced from github or the intermediate commits could be added or switch to git based off the commit of the last security update or later.
Comment by Sébastien Luttringer (seblu) - Saturday, 10 November 2018, 00:22 GMT
Reading the commits, looks like 3fcd042d26d70856e826a42b5f93dc4854d80bf0 is also security related.
If there no objection I will patch to latest commit.

Upstream should release after they publish a CVE.
Comment by Levente Polyak (anthraxx) - Saturday, 10 November 2018, 00:38 GMT
according to the svn tree its already patched up except of 3fcd042d26d70856e826a42b5f93dc4854d80bf0.

just pulling that one addition in would be preferred and would save the signature.
lets not kill signatures if not really required, patch normally doesn't require excessive backports so we can assume it will normalize soon
Comment by Sébastien Luttringer (seblu) - Saturday, 10 November 2018, 01:13 GMT
ok, I'll push a new version in testing with loqs patches and 3fcd042d26d70856e826a42b5f93dc4854d80bf0.

What do you means by saving the signature?
Comment by Sébastien Luttringer (seblu) - Saturday, 10 November 2018, 01:25 GMT
New package is in testing.

I didn't waste time dealing with patches conflicts because another dev has to commit to this package in the meantime.
Comment by Eli Schwartz (eschwartz) - Sunday, 11 November 2018, 00:42 GMT
> It takes files ending by .patch. What is mysterious heuristics?

As implemented inline in the PKGBUILD, it encourages PKGBUILDs to be more verbose than necessary -- it takes a seven-line prepare function to apply *one* patch, and only demonstrates usefulness when you routinely have more than seven patches to apply. When there are no patches to apply, it makes people do a double-take, then spend five minutes repeatedly reading the PKGBUILD trying to figure out where the patch is, that the prepare() function implies should exist.

Once I get past that, now in order to tell if a patch is applied properly, I have to read the snippet, figure out what conditions trigger it, then reread the source array, then try to figure out whether or not it works with renamed patches, and then rely on *watching comments* fly past during the "==> Starting prepare()..." step of makepkg in order to guarantee I've gotten it correct.

If I add an explicit patch command and the filename I try to apply, does not exist (maybe because I forgot to rename it to something ending in .patch, maybe because I commented out the name in the source array), then it will instantly fail and I'll know it.

Aside: this comment "should be a pacman feature" is IMO totally incorrect, for a couple reasons:
1) Nothing is a pacman feature unless it's proposed as such, and I cannot find any feature request for it,
2) If someone did propose it, then as a current major contributor to makepkg I would argue long and hard that it's a very very bad idea, not just because of the confusion I described above, but also because the excessive optimization towards build systems that try to read your mind and guess what you want to do, is exactly the reason I *don't* use the sort of distros that implement these so-called features. I consider it asking for trouble, no less than a build system that that tests to see if a Makefile exists somewhere and then automatically calls `make` without using a build() function.
3) As a personal choice, I would never use it, or demand the right to turn it off, so I could add inline comments to the prepare function before each patch, describing why it is necessary to modify upstream code. This is typically a link to some upstream pull request, or to a bugtracker ticket. Applying these comments to the source=() array would be confusing IMO, because there's no clear separation between patches and other content, and because it makes it harder to visually parse the different components of a PKGBUILD.

4) If it were to become a pacman feature, the absolute minimum criteria for it to be accepted is that it would have to support makepkg's documented source file renaming, which is used in many PKGBUILDs in the wild:

> It is also possible to change the name of the downloaded file, which is helpful with weird URLs and for handling multiple source files with the same name. The syntax is: source=('filename::url').

And it would also have to support architecture-specific sources, which also exist in many PKGBUILDs in the wild (pacman is not specific to Arch Linux) and which I believe are the correct way to handle x86_64 specific sources even in a distro that only supports x86_64.

Using makepkg's undocumented internal functions (get_all_sources_for_arch, get_filename, msg2, error) which we may change at any time, this would look something like:

do_patches() {
local netfile file all_sources ret=0
get_all_sources_for_arch 'all_sources'

for netfile in "${all_sources[@]}"; do
file=$(get_filename "$netfile")

if [[ $file = *.patch ]]; then
msg2 "Automatically applying patch '$file'..."
patch -p1 -i "$srcdir/$file" || ret=1
if (( ret != 0 )); then
error "Failed to automatically apply $file, make sure it is formatted in 'patch -p1' format."
return 1
fi
fi
done
}

Note: This would mean makepkg would forbid patches that aren't in -p1 format, and it would also need to support migration... this is just what you need to robustly support a PKGBUILD that we already assume is cooperating...
Comment by Sébastien Luttringer (seblu) - Sunday, 11 November 2018, 14:44 GMT
Woah :)

It's an helper to save me few time. I have to confess that I prefer have X static lines instead of copying twice the name of the patch to apply and add an echo.
I don't believe it takes 5 minutes to readers to understand what it does, and certainly not to someone like you, a « current major contributor to makepkg ».

We cannot expect our patch apply well. We may forget to add the patch line or miss one in a series. Most of the time I added a patch line which failed in the middle of other, I had to use extra echo lines to see which filename fails. So, helper or not, we need to read what happen in prepare().

This helper is far from perfect and my comment about the missing pacman feature doesn't means it should be added as is. It sucks. The feature, I was thinking of was support from makepkg to apply patches. Something like a patch prefix (patch+), like we do for vcs in the source array, which download patches (no matter how they end) and apply them (with error if not) before the prepare() function. The strip level could be a fragment (#1) like we do witch vcs.
I'm sorry I didn't get what you call support for migration.
Comment by Eli Schwartz (eschwartz) - Sunday, 11 November 2018, 22:13 GMT
My point is that it's not always so obvious how this should work, and it can be confusing to many people, especially if they don't know to look for it. Allan got confused too. :)

> It's an helper to save me few time. I have to confess that I prefer have X static lines instead of copying twice the name of the patch to apply and add an echo.

$ do:() { local - PS4='+ '; set -x; "$@"; }
$ do: patch -p1 -i "$srcdir"/foo.patch
+ patch -p1 -i /build/bar/src/foo.patch
patching file lib/libbar/main.c

(Or even `local -; set -x` in the prepare function itself, to print *every* command as it's run.)
This sounds more generically like the motivation that sees people adding msg2 traces for every command they run in every single function...

> I'm sorry I didn't get what you call support for migration.

If it were added to makepkg itself, would be incompatible with PKGBUILDs that use an existing prepare() function that would try to apply the patch a second time.
Comment by Eli Schwartz (eschwartz) - Sunday, 11 November 2018, 22:26 GMT
Anyway, we're currently applying all patches one way or another, yes? This is correctly fixed now?
Comment by Sébastien Luttringer (seblu) - Sunday, 11 November 2018, 22:39 GMT
Ok I see, that was about adding my crappy workaround.

A solution with sources+=(patch+https://allan.broke.it/patch?id=42#2) looks perfectly compatible with existing PKGBUILD. Are you still thinking it's a very very bad idea?
Comment by Levente Polyak (anthraxx) - Sunday, 11 November 2018, 22:49 GMT
if so, it should add the prefix value, like patch0+ patch1+ patch2+
anyway, this is not the place for such a discussion. please bring this to the pacman mailinglist if we want to have a conversation about this.
Comment by Sébastien Luttringer (seblu) - Monday, 12 November 2018, 17:59 GMT
Store the strip prefix in the "fragment" as defined in PKGBUILD vcs (patch+http://url#strip=1) is more elegant.
Sure, it's not the place to discuss pacman features, we are just dreaming here.

Patches are all applied on the sources. As they are cherry-pick of security patches, would be nice to have extra review of the patch content.
Currently there is only one signoff.

Comment by Levente Polyak (anthraxx) - Monday, 12 November 2018, 18:31 GMT
I just signed this off from two machines with multiple test sets so its moved now. We can finally resolve this
Comment by Sébastien Luttringer (seblu) - Monday, 12 November 2018, 21:28 GMT
Levante discovered that pkgrel=7 use incorrect patches that don't fix the issue (again).
He'll push a new version soon.

Side note, he accepted to co-maintain the package.
Comment by loqs (loqs) - Monday, 12 November 2018, 21:35 GMT
Should that be pkgrel=6 has an issue and the issue will be resolved in pkgrel=7 ?
Comment by Sébastien Luttringer (seblu) - Monday, 12 November 2018, 21:41 GMT
Correct. I was anticipating the next mistake. :p
Comment by Levente Polyak (anthraxx) - Monday, 12 November 2018, 21:42 GMT
Yes, please have a look at my patch before we spot something again and need a -8 it explains what has been changed:
https://github.com/anthraxx/arch-pkgbuilds/commit/6b3f8202925ab3855b8417e56b6e63b4c15619c0

The patchset was a bit mixed up and weird, it contained hunks of other patches and it was totally in-transparent plus it actually undid CVE-2018-6952 which was applied before.

Therefor i just removed everything, used unchaged upstream patches where possible and just needed to change a single commit (that is in tree) and there only adjust 2 context lines in a single hunk so all patches are as upstream provides them.
Comment by loqs (loqs) - Monday, 12 November 2018, 21:55 GMT
I deeply apologize for the mistakes in the patchset especially as it relates to a security issue.
Comment by Levente Polyak (anthraxx) - Monday, 12 November 2018, 21:58 GMT
no worries, issues happen. we should have spotted that earlier but luckily seblu tossed me into doing a full pedantic review :D

Just please everyone take a look and test the version on my GH so we can finally build a version that has everything in place :)

Loading...