Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[21.05] hedgedoc: 1.8.2 -> 1.9.0, fixes CVE-2021-39175 #139238

Merged
merged 3 commits into from
Sep 28, 2021
Merged

[21.05] hedgedoc: 1.8.2 -> 1.9.0, fixes CVE-2021-39175 #139238

merged 3 commits into from
Sep 28, 2021

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Sep 23, 2021 •
edited
Loading

Motivation for this change

Backport of #138468.

ChangeLog: https://github.com/hedgedoc/hedgedoc/releases/tag/1.9.0

As documented in the Nix expression, I unfortunately had to patch
yarn.lock manually (the yarn.nix result isn't affected by this). By
adding a git+https-prefix to
midi "https://github.com/paulrosen/MIDI.js.git#abcjs" in the lock-file
I ensured that yarn actually uses the MIDI.js from the offline-cache
from yarn2nix rather than trying to download a tarball from GitHub.

Also, this release contains a fix for CVE-2021-39175 which doesn't seem
to be backported to 1.8. To quote NVD[1]:

In versions prior to 1.9.0, an unauthenticated attacker can inject
arbitrary JavaScript into the speaker-notes of the slide-mode feature
by embedding an iframe hosting the malicious code into the slides or by
embedding the HedgeDoc instance into another page.

Even though it "only" has a medium rating by NVD (6.1), this seems
rather problematic to me (also, GitHub rates this as "High"), so it's
actually a candidate for a backport.

[1] https://nvd.nist.gov/vuln/detail/CVE-2021-39175

(cherry picked from commit 0a10c17)

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@Ma27
hedgedoc: 1.8.2 -> 1.9.0, fixes CVE-2021-39175
4eb0a2b 
ChangeLog: https://github.com/hedgedoc/hedgedoc/releases/tag/1.9.0

As documented in the Nix expression, I unfortunately had to patch
`yarn.lock` manually (the `yarn.nix` result isn't affected by this). By
adding a `git+https`-prefix to
`midi "https://github.com/paulrosen/MIDI.js.git#abcjs"` in the lock-file
I ensured that `yarn` actually uses the `MIDI.js` from the offline-cache
from `yarn2nix` rather than trying to download a tarball from GitHub.

Also, this release contains a fix for CVE-2021-39175 which doesn't seem
to be backported to 1.8. To quote NVD[1]:

> In versions prior to 1.9.0, an unauthenticated attacker can inject
> arbitrary JavaScript into the speaker-notes of the slide-mode feature
> by embedding an iframe hosting the malicious code into the slides or by
> embedding the HedgeDoc instance into another page.

Even though it "only" has a medium rating by NVD (6.1), this seems
rather problematic to me (also, GitHub rates this as "High"), so it's
actually a candidate for a backport.

[1] https://nvd.nist.gov/vuln/detail/CVE-2021-39175

(cherry picked from commit 0a10c17)
@Ma27 Ma27 requested a review from WilliButz September 23, 2021 22:06
@ofborg ofborg bot requested a review from globin September 23, 2021 22:17
@r-rmcgibbo
Copy link

r-rmcgibbo commented Sep 23, 2021 •
edited
Loading

Result of nixpkgs-review pr 139238 at 4eb0a2b run on aarch64-linux 1

1 package failed to build:
1 package built successfully:
  • nixos-install-tools

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

Result of nixpkgs-review pr 139238 at 4eb0a2b run on x86_64-linux 1

1 package failed to build:
1 package built successfully:
  • nixos-install-tools
4 suggestions:
  • warning: build-tools-in-build-inputs

    rsync is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/development/tools/yarn2nix-moretea/yarn2nix/default.nix:292:7:

        |
292 |       buildInputs = [ yarn nodejs rsync ] ++ extraBuildInputs;
    |       ^
  • warning: unused-argument

    Unused argument: fetchpatch.
    Near pkgs/servers/web-apps/hedgedoc/default.nix:4:3:

      |
4 | , fetchpatch
  |   ^
  • warning: unclear-gpl

    agpl3 is a deprecated license, please check if project uses agpl3Plus or agpl3Only and change meta.license accordingly.

    Near pkgs/servers/web-apps/hedgedoc/default.nix:108:5:

        |
108 |     license = licenses.agpl3;
    |     ^
  • warning: build-tools-in-build-inputs

    yarn is a build tool so it likely goes to nativeBuildInputs, not buildInputs.

    Near pkgs/development/tools/yarn2nix-moretea/yarn2nix/default.nix:292:7:

        |
292 |       buildInputs = [ yarn nodejs rsync ] ++ extraBuildInputs;
    |       ^

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement. Does something look off? Please file an issue or reach out on IRC.

@Ma27
Copy link
Member Author

Ma27 commented Sep 23, 2021

Meh. Gonna regenerate the lockfiles tomorrow.

@Ma27
Copy link
Member Author

Ma27 commented Sep 24, 2021

@ofborg build hedgedoc

Rebuilding now. I could reproduce the hash-mismatch, but it's gone now. If I had to guess, it's GitHub where something got screwed up badly.

@Ma27
Copy link
Member Author

Ma27 commented Sep 24, 2021

Now this is weird. Can anybody please check the result of nix-build /nix/store/zr0dr0cczfgbdyl3y08mipv90wz8xl52-js-sequence-diagrams-bda0e49.drv --check here?

yayayayaka
yayayayaka reviewed Sep 25, 2021
View reviewed changes
Copy link
Member

@yayayayaka yayayayaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to build your PR and it fails with

mak@build-worker-01:~/nixpkgs $ nix build -f . hedgedoc
hash mismatch in fixed-output derivation '/nix/store/2j7bifz3c5spf838gkxz6lhv3406rzjq-js-sequence-diagrams-bda0e49':
  wanted: sha256:0rl29jmhv7vhadzb6d08hi9g64227r9j10fh3d0lbgxinrib5gma
  got:    sha256:0d2zf62fmad760rg9hrkyhp03k5apms3fm0mf64yy8q6p3iw7jvw
cannot build derivation '/nix/store/rq2mnls3gdql1z2xlc8lwl8dgf2jccgf-js-sequence-diagrams.git.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/qwwn2ip9w232is822vkz0gnblhaxwzrj-offline.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/716sxkd1gbbac8kzl0v0shn9csfn4a4z-HedgeDoc-modules-1.9.0.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/rnyg66ir8py1lvih5p9xk7x8f3pcxqm8-HedgeDoc-1.9.0.drv': 1 dependencies couldn't be built
[38 built (1 failed), 907 copied (108.7 MiB), 91.7 MiB DL]
error: build of '/nix/store/rnyg66ir8py1lvih5p9xk7x8f3pcxqm8-HedgeDoc-1.9.0.drv' failed
mak@build-worker-01:~/nixpkgs $ nix-build /nix/store/zr0dr0cczfgbdyl3y08mipv90wz8xl52-js-sequence-diagrams-bda0e49.drv --check
these derivations will be built:
  /nix/store/zr0dr0cczfgbdyl3y08mipv90wz8xl52-js-sequence-diagrams-bda0e49.drv
error: some outputs of '/nix/store/zr0dr0cczfgbdyl3y08mipv90wz8xl52-js-sequence-diagrams-bda0e49.drv' are not valid, so checking is not possible
mak@build-worker-01:~/nixpkgs $

@Ma27
Copy link
Member Author

Ma27 commented Sep 25, 2021

Yeah, as mentioned above, I'm aware of that (also, ofborg already failed with the same issue) :)

Will try to investigate what's wrong at some point.

@Ma27
Copy link
Member Author

Ma27 commented Sep 25, 2021

OK, the overall cause is that the store-path with the new hash (i.e. the store-path that doesn't mismatch) is missing a submodule while the problematic store-path doesn't.

However, both stem from the same derivation (/nix/store/zr0dr0cczfgbdyl3y08mipv90wz8xl52-js-sequence-diagrams-bda0e49.drv). Not sure how that's possible so far, but I'll see.

@Ma27
Copy link
Member Author

Ma27 commented Sep 25, 2021

OK, found the problem (and with this explanation, master is also affected, so I'll fix that first):

  • the first part is rather trivial: yarn2nix runs nix-prefetch-git without --fetch-submodules, but fetchgit fetches submodule sby default (I think that this behavior is really problematic btw). The easiest fix is to change the nix-prefetch-git of yarn2nix. COnsidering that noone else had this before, this just wasn't noticed until now.
  • It took me a while until I realized that I still had some output path with the wrong hash lying around which silenced the error. HOWEVER, nix-build --check didn't raise an error even though it should which seems like a Nix bug to me (in fact, I would've noticed way earlier otherwise), will file a patch for that later.

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Sep 25, 2021
@Ma27
yarn2nix: run nix-prefetch-git with --fetch-submodules
46c98ae 
`pkgs.fetchgit` uses `fetchSubmodules = true;` by default, however
`nix-prefetch-git` doesn't. This means that hashes for a Git repository
with fetched submodules will be wrong in `yarn.nix`.

Considering that this got unnoticed before, it seems as if this case is
an exception to a certain degree.

An exemplary problem is the last `hedgedoc` update[1] where
`js-sequence-diagrams` - a Git repo with submodules - from upstream's
package.json caused a hash mismatch. This got unnoticed because
`nix-build --check` doesn't seem to reveal these issues for fixed-output
derivations.

[1] NixOS#139238
@Ma27
yarn2nix: run nix-prefetch-git with --fetch-submodules
c20fbab 
`pkgs.fetchgit` uses `fetchSubmodules = true;` by default, however
`nix-prefetch-git` doesn't. This means that hashes for a Git repository
with fetched submodules will be wrong in `yarn.nix`.

Considering that this got unnoticed before, it seems as if this case is
an exception to a certain degree.

An exemplary problem is the last `hedgedoc` update[1] where
`js-sequence-diagrams` - a Git repo with submodules - from upstream's
package.json caused a hash mismatch. This got unnoticed because
`nix-build --check` doesn't seem to reveal these issues for fixed-output
derivations.

[1] NixOS#139238

(cherry picked from commit 46c98ae)
@Ma27
hedgedoc: fix build by re-running yarn2nix
36a3756 
Failing Hydra build: https://hydra.nixos.org/build/154209534

(cherry picked from commit 0a2615f)
@Ma27
Copy link
Member Author

Ma27 commented Sep 25, 2021

This now contains the changes from #139489 as well, so we should wait until this is also merged :)

@yayayayaka
Copy link
Member

I could reproduce the hash-mismatch, but it's gone now.

That sentence led me to believe the issue had been fixed at that point.

@Ma27
Copy link
Member Author

Ma27 commented Sep 26, 2021

Oh I only gave an update that it wasn't gone in Matrix, but not here, sorry for that! :)

Ma27 added a commit to Ma27/nix that referenced this pull request Sep 26, 2021
@Ma27
build: also throw hash-mismatch errors if buildMode == bmCheck
391bf3d 
This actually bit me quite recently in `nixpkgs` because I assumed that
`nix-build --check` would also error out if hashes don't match anymore[1]
and so I wrongly assumed that I couldn't reproduce the mismatch error.

The fix is rather simple, during the output registration a so-called
`delayedException` is instantiated e.g. if a FOD hash-mismatch occurs.
However, in case of `nix-build --check` (or `--rebuild` in case of `nix
build`), the code-path where this exception is thrown will never be
reached.

By adding that check to the if-clause that causes an early exit in case
of `bmCheck`, the issue is gone. Also added a (previously failing)
test-case to demonstrate the problem.

[1] NixOS/nixpkgs#139238, the underlying issue
    was that `nix-prefetch-git` returns different hashes than `fetchgit`
    because the latter one fetches submodules by default.
@Ma27 Ma27 mentioned this pull request Sep 26, 2021
Merged
yu-re-ka pushed a commit that referenced this pull request Sep 26, 2021
@Ma27 @yu-re-ka
yarn2nix: run nix-prefetch-git with --fetch-submodules
ca4e61d 
`pkgs.fetchgit` uses `fetchSubmodules = true;` by default, however
`nix-prefetch-git` doesn't. This means that hashes for a Git repository
with fetched submodules will be wrong in `yarn.nix`.

Considering that this got unnoticed before, it seems as if this case is
an exception to a certain degree.

An exemplary problem is the last `hedgedoc` update[1] where
`js-sequence-diagrams` - a Git repo with submodules - from upstream's
package.json caused a hash mismatch. This got unnoticed because
`nix-build --check` doesn't seem to reveal these issues for fixed-output
derivations.

[1] #139238
Ma27 added a commit to Ma27/nix that referenced this pull request Sep 27, 2021
@Ma27
build: also throw hash-mismatch errors if buildMode == bmCheck
44440a5 
This actually bit me quite recently in `nixpkgs` because I assumed that
`nix-build --check` would also error out if hashes don't match anymore[1]
and so I wrongly assumed that I couldn't reproduce the mismatch error.

The fix is rather simple, during the output registration a so-called
`delayedException` is instantiated e.g. if a FOD hash-mismatch occurs.
However, in case of `nix-build --check` (or `--rebuild` in case of `nix
build`), the code-path where this exception is thrown will never be
reached.

By adding that check to the if-clause that causes an early exit in case
of `bmCheck`, the issue is gone. Also added a (previously failing)
test-case to demonstrate the problem.

[1] NixOS/nixpkgs#139238, the underlying issue
    was that `nix-prefetch-git` returns different hashes than `fetchgit`
    because the latter one fetches submodules by default.
Ma27 added a commit to Ma27/nix that referenced this pull request Sep 27, 2021
@Ma27
build: also throw hash-mismatch errors if buildMode == bmCheck
bb1a851 
This actually bit me quite recently in `nixpkgs` because I assumed that
`nix-build --check` would also error out if hashes don't match anymore[1]
and so I wrongly assumed that I couldn't reproduce the mismatch error.

The fix is rather simple, during the output registration a so-called
`delayedException` is instantiated e.g. if a FOD hash-mismatch occurs.
However, in case of `nix-build --check` (or `--rebuild` in case of `nix
build`), the code-path where this exception is thrown will never be
reached.

By adding that check to the if-clause that causes an early exit in case
of `bmCheck`, the issue is gone. Also added a (previously failing)
test-case to demonstrate the problem.

[1] NixOS/nixpkgs#139238, the underlying issue
    was that `nix-prefetch-git` returns different hashes than `fetchgit`
    because the latter one fetches submodules by default.
@Ma27 Ma27 merged commit ce6b39b into NixOS:release-21.05 Sep 28, 2021
@Ma27 Ma27 deleted the backport-hedgedoc branch September 28, 2021 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yayayayaka yayayayaka yayayayaka left review comments

@WilliButz WilliButz Awaiting requested review from WilliButz

@globin globin Awaiting requested review from globin

Assignees
No one assigned
Labels
1.severity: security 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ma27 @r-rmcgibbo @yayayayaka @risicle