Arch Linux

I didn't find a patch for CVE-2018-6952. Will wait for both patch before push.

I'm correct if I says there is still no fix taken by upstream for CVE-2018-6952?

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.

hey guys!

All patches are upstream:
- 53132 https://github.com/mirror/patch/commit/f290f48a621867084884bfff87f8093c15195e6a
- 53133 https://github.com/mirror/patch/commit/9c986353e420ead6e706262bf204d6e03322c300
- CVE-2018-1000156 https://github.com/mirror/patch/commit/123eaff0d5d1aebe128295959435b9ca5909c26d

Sorry for the GitHub link but looks like savannah is down for me :-/

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

It takes files ending by .patch. What is mysterious heuristics?

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.

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.

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.

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.

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

ok, I'll push a new version in testing with loqs patches and 3fcd042d26d70856e826a42b5f93dc4854d80bf0.

What do you means by saving the signature?

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.

> 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...

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.

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.

Anyway, we're currently applying all patches one way or another, yes? This is correctly fixed now?

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?

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.

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.

I just signed this off from two machines with multiple test sets so its moved now. We can finally resolve this

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.

Should that be pkgrel=6 has an issue and the issue will be resolved in pkgrel=7 ?

Correct. I was anticipating the next mistake. :p

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.

I deeply apologize for the mistakes in the patchset especially as it relates to a security issue.

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 :)