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.

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

fix: Prevent relative install destinations leaking outside package install dir #8350

Merged
merged 1 commit into from
Aug 21, 2023

Conversation

gridbugs
Copy link
Collaborator

@gridbugs gridbugs commented Aug 7, 2023

This disallows install dst paths from begining with ".." to prevent them referencing a directory outside the install directories of the package.

This new constraint would prevent globs from being able to refer to files outside the current directory and its descendants, so this also adds a new keyword which changes the prefix of paths matched by a glob in the install stanza. It's similar to the (files (foo as bar)) syntax in that it allows installed files to be placed in a location other than the one implied by their position in the source tree. It's now possible to write:
(files (glob_files (some/path/* with_prefix new/path))) ...which takes all the paths matched by the glob and replaces the some/path component with new/path.

I haven't yet updated the docs or changelog or written any tests beside modifying the one defined in #8265 to demonstrate how the feature gets used to give us a chance to discuss and change things about this solution before taking the time to write docs/tests, etc.

Also note that this breaks the test github2123. I followed the rabbit hole starting from #2123 to understand why this is necessary but I still don't understand. The test uses this dune file:

(install
 (section lib)
 (files (mirage-xen.pc as ../pkgconfig/mirage-xen.pc)))

That file ends up in _build/install/default/lib/pkgconfig/mirage-xen.pc which seems like it's escaping the package's install directory by design. @rgrinberg is that right? That issue was made in 2019. Do we know if mirage still needs this feature?

@gridbugs gridbugs requested a review from rgrinberg August 7, 2023 08:41
@Leonidas-from-XIV
Copy link
Collaborator

If I understand it correctly, the idea is that Mirage installs a its own pkg-config file into a global configuration. On my system according to pkg-config --dump-personality (amusing CLI argument of the day) it searches in /usr/lib64/pkgconfig and /usr/share/pkgconfig so all the .pc files need to be dumped in a common folder where pkg-config will be looking it up.

For this specific use case we could introduce a way to install .pc files, but I wonder if there are other projects that also legitimately install files in ../* locations that would break - my guess is that a lot of projects do that so it might end up breaking quite a lot of builds.

@Alizter
Copy link
Collaborator

Alizter commented Aug 10, 2023

Can we get around with this constraint using symlinks?

@gridbugs
Copy link
Collaborator Author

Can we get around with this constraint using symlinks?

How would that work? Could we allow files outside their package directory provided that they are symlinks?

@Leonidas-from-XIV
Copy link
Collaborator

How would that work? Could we allow files outside their package directory provided that they are symlinks?

I guess if you symlink .. to foo in your source tree you could write foo/bar and have bar end up in the parent folder (not that I tested it). Also not sure what happens if you write foo/bar to an .install file, whether OPAM will create symlink directories and write through symlink directories. It all feels very "exploity".

@rgrinberg
Copy link
Member

@gridbugs the fix looks right to me. However, it has to be guarded behind a dune language version. Before we remove the old behavior, we need to go through a deprecation cycle as well.

That file ends up in _build/install/default/lib/pkgconfig/mirage-xen.pc which seems like it's escaping the package's install directory by design. @rgrinberg is that right? That issue was made in 2019. Do we know if mirage still needs this feature?

I don't know about mirage, but installing pkg-config metadata is something that I'm sure many people are going to need.

@kit-ty-kate what is the recommended way to install such files?

@kit-ty-kate
Copy link
Member

@kit-ty-kate what is the recommended way to install such files?

I think using lib_root instead of lib would be the recommended way to do this.

@rgrinberg
Copy link
Member

Okay, thanks. So we can make the deprecation message include the suggestion to use this Lib_root section (not in this PR)

@gridbugs
Copy link
Collaborator Author

gridbugs commented Aug 15, 2023

Ok so for this PR I'll make it so that the current behaviour is maintained prior to the the next release of dune after which we'll start printing a deprecated message (but it won't be an error).

Should I mention the deprecation in the docs or should the docs just say that dst paths may not begin with ".."?

@gridbugs gridbugs force-pushed the glob_files_rec-fix branch 2 times, most recently from ee9509a to d2c8440 Compare August 15, 2023 08:57
@gridbugs
Copy link
Collaborator Author

I've updated this change to only print a warning rather than an error and only when the dune lang version is >= 3.11. Still need to add docs and tests and update changelog.

Also a newly added test is triggering the warning:

  $ test "(mydocs as ../)"
  lib: [
    "_build/install/default/lib/mypkg/META"
    "_build/install/default/lib/mypkg/dune-package"
  ]
  doc: [
    "_build/install/default/doc/baz.md" {"../baz.md"}
    "_build/install/default/doc/foo.md" {"../foo.md"}
    "_build/install/default/doc/foo/bar.md" {"../foo/bar.md"}
  ]

@rgrinberg this is in the test for installing source trees and it looks like this test is deliberately escaping its install directory (everything should be under .../default/doc/mypkg/). Is that intended?

@rgrinberg
Copy link
Member

this is in the test for installing source trees and it looks like this test is deliberately escaping its install directory (everything should be under .../default/doc/mypkg/). Is that intended?

The test is indeed intentionally testing this behavior. For source_trees, we can just disable escaping the section root out right since this feature hasn't been introduced.

@rgrinberg
Copy link
Member

Should I mention the deprecation in the docs or should the docs just say that dst paths may not begin with ".."?

I think you should mention it. It's useful information for the user, so of course it can only help if it's in the manual.

I wish we more systematic about mentioning versions in the manual as well. This would save users a bunch of time.

@gridbugs gridbugs force-pushed the glob_files_rec-fix branch 3 times, most recently from 13d7944 to 8d013bd Compare August 16, 2023 08:19
@gridbugs gridbugs requested a review from christinerose August 16, 2023 08:20
@gridbugs
Copy link
Collaborator Author

Starting a dst with .. is now an error when installing source trees and a warning when installing files and dirs. I've added a bunch of tests of this behaviour and also of the with_prefix feature when using globs in the install stanza. I added docs for both features as well and also ended up adding more docs and tests for installing source trees to clarify some edge cases.

Added @christinerose to review the documentation changes.

@gridbugs gridbugs force-pushed the glob_files_rec-fix branch from 8d013bd to edb9cd2 Compare August 16, 2023 09:04
@Alizter
Copy link
Collaborator

Alizter commented Aug 16, 2023

I'm still a big confused about the motivation. Why do we want to restrict install directories? That seems like we will be more opinionated than we need to be.

I am of the opinion that installs should be atomic and self-contained, however I feel like this restriction might be raising the barrier for projects with strange install requirements. I don't think we should exclude such projects from using Dune since we don't know what their reasons might be.

On the other hand, I can see that this might be useful for the package management work. However in that case, we cannot place any such restriction on make builds so maybe a more general "safety net" for install is needed other than just checking for ... This is of course if I have understood the motivation correctly.

@rgrinberg rgrinberg added this to the 3.11.0 milestone Aug 16, 2023
@rgrinberg
Copy link
Member

The motivation is that doing section xxx and then using ../ will actually escape outside of the section. That's almost never intentional, and if there's a need for it, there should be a proper way of accomplishing this.

@gridbugs gridbugs force-pushed the glob_files_rec-fix branch from edb9cd2 to e61cdeb Compare August 17, 2023 04:56
let open Syntax.Version.Infix in
if dune_syntax >= (3, 11)
then
User_warning.emit
Copy link
Member

Choose a reason for hiding this comment

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

Warnings should be disabled in vendored projects.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand this comment. What are you suggesting I change here?

Copy link
Member

Choose a reason for hiding this comment

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

In a project that's under a (vendored_dirs ..), we omit all the warnings. This is so that projects that emit projects used by opam-monorepo (or vendored otherwise), do not spam the main projects with warnings.

@rgrinberg
Copy link
Member

Looks good to me with a few minor suggestions remaining.

I don't know if this possible at this point, but I would have liked with_prefix to come in a subsequent PR.

Also, I think you need to add a few test cases to make sure with_prefix is being validated correctly. For example, absolute paths shouldn't be allowed there.

@Leonidas-from-XIV could you give this one some review as well?

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

While I am not sure this is a good idea from a compatibility point of view (I guess this could prevent a number of packages upgrading to dune-lang 3.11 since we take away something without providing a clear way to migrate to) the code is fine. Some minor comments attached.

doc/stanzas/install.rst Outdated Show resolved Hide resolved
doc/stanzas/install.rst Outdated Show resolved Hide resolved
doc/stanzas/install.rst Outdated Show resolved Hide resolved
src/dune_rules/glob_files_expand.ml Outdated Show resolved Hide resolved
src/dune_rules/install_entry.ml Outdated Show resolved Hide resolved
@gridbugs
Copy link
Collaborator Author

While I am not sure this is a good idea from a compatibility point of view (I guess this could prevent a number of packages upgrading to dune-lang 3.11 since we take away something without providing a clear way to migrate to) the code is fine. Some minor comments attached.

Regarding compatibility I can take a look through the monorepo benchmark duniverse to see how many packages take advantage of the as keyword to escape their install directory. We know that some packages do this to add pkg-config entries but it would be interesting to see if there are any other use cases that will be affected by this change.

For pkg-config the migration is to use the lib_root section. Where's the right place to add migration notes? Should it go in the changelog?

@gridbugs gridbugs force-pushed the glob_files_rec-fix branch 3 times, most recently from 9e802a9 to 78b63c5 Compare August 18, 2023 05:20
@gridbugs
Copy link
Collaborator Author

I don't know if this possible at this point, but I would have liked with_prefix to come in a subsequent PR.

I've reduced this PR to just the deprecation of dsts starting with "..". I'll move the rest to a new PR.

@gridbugs
Copy link
Collaborator Author

I moved the with_prefix change to here: #8416

@rgrinberg
Copy link
Member

@gridbugs I rebased the PR. I think it's still showing up warnings in vendored directories. It should be silenced there.

This disallows install dst paths from begining with ".." to prevent them
referencing a directory outside the install directories of the package.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@gridbugs gridbugs force-pushed the glob_files_rec-fix branch from 4f786e1 to 8ab9938 Compare August 21, 2023 04:49
@gridbugs
Copy link
Collaborator Author

@rgrinberg if I'm correctly understanding you regarding vendored directories the problem would only happen if dune tried to build the install manifest for a vendored project. I don't know if it's possible for this to happen. I added a test to the end of test/blackbox-tests/test-cases/start-install-dst-with-parent-error.t that I thought might trigger this but it didn't work. Any idea how make dune evaluate the install stanza inside a vendored project?

@rgrinberg
Copy link
Member

You're right of course. We disable install rules for vendored packages anyway. Anyway, it's just something to keep in mind in general then.

@rgrinberg rgrinberg changed the title Prevent relative install destinations leaking outside package install dir fix: Prevent relative install destinations leaking outside package install dir Aug 21, 2023
@rgrinberg rgrinberg merged commit 86d7ab1 into ocaml:main Aug 21, 2023
emillon added a commit to emillon/opam-repository that referenced this pull request Sep 14, 2023
CHANGES:

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will raise an error. (ocaml/dune#7674, @Alizter)

- `enabled_if` now supports `arch_sixtyfour` variable (ocaml/dune#8023, fixes ocaml/dune#7997,
  @Alizter)- Use `posix_spawn` instead of `fork` on MacOS. This gives us a
  performance boost and allows us to re-enable thread. (ocaml/dune#8090, @rgrinberg)

- Experimental: Added a `$ dune monitor` command that can connect to a running
  `dune build` in watch mode and display the errors and progress. (ocaml/dune#8152,
  @Alizter)

- No longer emit linkopts(javascript) in META files (ocaml/dune#8168, @hhugo)

- The `progress` RPC procedure now has an extra field for the `In_progress`
  constructor for the number of failed jobs. (ocaml/dune#8212, @Alizter)

- Add a `--preview` flag to `dune fmt` which causes it to print out the changes
  it would make without applying them (ocaml/dune#8289, @gridbugs)

- Introduce `(source_trees ..)` to the install stanza to allow installing
  entire source trees. (ocaml/dune#8349, @rgrinberg)

- Deprecate install destination paths beginning with ".." to prevent packages
  escaping their designated installation directories. (ocaml/dune#8350, @gridbugs)

- Stop signing source files with substitutions. Sign only binaries instead
  (ocaml/dune#8361, fixes ocaml/dune#8360, @anmonteiro)

- Add `--stop-on-first-error` option to `dune build` which will terminate the
  build when the first error is encountered. (ocaml/dune#8400, @pmwhite and @Alizter)-
  Dune now displays the number of errors when waiting for changes in watch
  mode. (ocaml/dune#8408, fixes ocaml/dune#6889, @Alizter)

- Add `with_prefix` keyword for changing the prefix of the destination of
  installed files matched by globs. (ocaml/dune#8416, @gridbugs)

- Added experimental `--display tui` option for Dune that opens an interactive
  Terminal User Interface (TUI) when Dune is running. Press '?' to open up a
  help screen when running for more information. (ocaml/dune#8429, @Alizter and
  @rgrinberg)

- Add a `warnings` field to `dune-project` files as a unified mechanism to
  enable or disable dune warnings (@rgrinberg, 8448)

- `dune exec`: support syntax like `%{bin:program}`. This can appear anywhere
  in the command line, so things like `dune exec time %{bin:program}` now work.
  (ocaml/dune#6035, ocaml/dune#8474, fixes ocaml/dune#2691, @emillon, @Leonidas-from-XIV)

- RPC message styles are now serialised meaning that RPC diagnostics keep their
  Ansi styling. (ocaml/dune#8516, fixes ocaml/dune#6921, @Alizter)- Ignore internal promote rules
  when `--ignore-promoted-rules` is set (ocaml/dune#8518, fix ocaml/dune#8417, @rgrinberg)

- Truncate output from actions that produce too much output (@tov, ocaml/dune#8351)

- Allow libraries to shadow OCaml builtin libraries. Previously, builtin
  libraries would always take precedence. (@rgrinberg, ocaml/dune#8558)

- Remove warning against `.dune` files generated by pre dune 2.0 (ocaml/dune#8611,
  @rgrinberg)

- `dune utop` no longer links `utop` in "custom" mode, which should make this
  command considerably faster. (ocaml/dune#8631, fixes ocaml/dune#6894, @nojb)

- Ensure that package names in `dune-project` are valid opam package
  names. (ocaml/dune#8331, @emillon)

- dune init: parse `--public` as a public name (ocaml/dune#8603, fixes ocaml/dune#7108, @emillon)

- Remove versions 0.1 and 0.2 of the experimental ctypes extension. (ocaml/dune#8293,
  @emillon)
emillon added a commit to emillon/opam-repository that referenced this pull request Sep 22, 2023
CHANGES:

- `enabled_if` now supports `arch_sixtyfour` variable (ocaml/dune#8023, fixes ocaml/dune#7997,
  @Alizter)

- Use `posix_spawn` instead of `fork` on MacOS. This gives us a performance
  boost and allows us to re-enable thread. (ocaml/dune#8090, @rgrinberg)

- Experimental: Added a `$ dune monitor` command that can connect to a running
  `dune build` in watch mode and display the errors and progress. (ocaml/dune#8152,
  @Alizter)

- The `progress` RPC procedure now has an extra field for the `In_progress`
  constructor for the number of failed jobs. (ocaml/dune#8212, @Alizter)

- Add a `--preview` flag to `dune fmt` which causes it to print out the changes
  it would make without applying them (ocaml/dune#8289, @gridbugs)

- Introduce `(source_trees ..)` to the install stanza to allow installing
  entire source trees. (ocaml/dune#8349, @rgrinberg)

- Add `--stop-on-first-error` option to `dune build` which will terminate the
  build when the first error is encountered. (ocaml/dune#8400, @pmwhite and @Alizter)

- Dune now displays the number of errors when waiting for changes in watch
  mode. (ocaml/dune#8408, fixes ocaml/dune#6889, @Alizter)

- Add `with_prefix` keyword for changing the prefix of the destination of
  installed files matched by globs. (ocaml/dune#8416, @gridbugs)

- Added experimental `--display tui` option for Dune that opens an interactive
  Terminal User Interface (TUI) when Dune is running. Press '?' to open up a
  help screen when running for more information. (ocaml/dune#8429, @Alizter and
  @rgrinberg)

- Add a `warnings` field to `dune-project` files as a unified mechanism to
  enable or disable dune warnings (@rgrinberg, 8448)

- `dune exec`: support syntax like `%{bin:program}`. This can appear anywhere
  in the command line, so things like `dune exec time %{bin:program}` now work.
  (ocaml/dune#6035, ocaml/dune#8474, fixes ocaml/dune#2691, @emillon, @Leonidas-from-XIV)

- Make copy sandbox support directory targets. (ocaml/dune#8705, fixes ocaml/dune#7724, @emillon)

- Add a new alias `@doc-json` to build odoc documentation in JSON format. This
  output can be consumed by external tools. (ocaml/dune#8178, @emillon)

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will raise an error. (ocaml/dune#7674, @Alizter)

- No longer emit linkopts(javascript) in META files (ocaml/dune#8168, @hhugo)

- Deprecate install destination paths beginning with ".." to prevent packages
  escaping their designated installation directories. (ocaml/dune#8350, @gridbugs)

- RPC message styles are now serialised meaning that RPC diagnostics keep their
  Ansi styling. (ocaml/dune#8516, fixes ocaml/dune#6921, @Alizter)

- Truncate output from actions that produce too much output (@tov, ocaml/dune#8351)

- Allow libraries to shadow OCaml builtin libraries. Previously, builtin
  libraries would always take precedence. (@rgrinberg, ocaml/dune#8558)

- Remove warning against `.dune` files generated by pre dune 2.0 (ocaml/dune#8611,
  @rgrinberg)

- `dune utop` no longer links `utop` in "custom" mode, which should make this
  command considerably faster. (ocaml/dune#8631, fixes ocaml/dune#6894, @nojb)

- Ensure that package names in `dune-project` are valid opam package names.
  (ocaml/dune#8331, @emillon)

- init: check that module names are valid (ocaml/dune#8644, fixes ocaml/dune#8252, @emillon)

- dune init: parse `--public` as a public name (ocaml/dune#8603, fixes ocaml/dune#7108, @emillon)

- Stop signing source files with substitutions. Sign only binaries instead
  (ocaml/dune#8361, fixes ocaml/dune#8360, @anmonteiro)

- Remove versions 0.1 and 0.2 of the experimental ctypes extension. (ocaml/dune#8293,
  @emillon)
@gridbugs gridbugs deleted the glob_files_rec-fix branch October 11, 2023 03:44
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

- `enabled_if` now supports `arch_sixtyfour` variable (ocaml/dune#8023, fixes ocaml/dune#7997,
  @Alizter)

- Use `posix_spawn` instead of `fork` on MacOS. This gives us a performance
  boost and allows us to re-enable thread. (ocaml/dune#8090, @rgrinberg)

- Experimental: Added a `$ dune monitor` command that can connect to a running
  `dune build` in watch mode and display the errors and progress. (ocaml/dune#8152,
  @Alizter)

- The `progress` RPC procedure now has an extra field for the `In_progress`
  constructor for the number of failed jobs. (ocaml/dune#8212, @Alizter)

- Add a `--preview` flag to `dune fmt` which causes it to print out the changes
  it would make without applying them (ocaml/dune#8289, @gridbugs)

- Introduce `(source_trees ..)` to the install stanza to allow installing
  entire source trees. (ocaml/dune#8349, @rgrinberg)

- Add `--stop-on-first-error` option to `dune build` which will terminate the
  build when the first error is encountered. (ocaml/dune#8400, @pmwhite and @Alizter)

- Dune now displays the number of errors when waiting for changes in watch
  mode. (ocaml/dune#8408, fixes ocaml/dune#6889, @Alizter)

- Add `with_prefix` keyword for changing the prefix of the destination of
  installed files matched by globs. (ocaml/dune#8416, @gridbugs)

- Added experimental `--display tui` option for Dune that opens an interactive
  Terminal User Interface (TUI) when Dune is running. Press '?' to open up a
  help screen when running for more information. (ocaml/dune#8429, @Alizter and
  @rgrinberg)

- Add a `warnings` field to `dune-project` files as a unified mechanism to
  enable or disable dune warnings (@rgrinberg, 8448)

- `dune exec`: support syntax like `%{bin:program}`. This can appear anywhere
  in the command line, so things like `dune exec time %{bin:program}` now work.
  (ocaml/dune#6035, ocaml/dune#8474, fixes ocaml/dune#2691, @emillon, @Leonidas-from-XIV)

- Make copy sandbox support directory targets. (ocaml/dune#8705, fixes ocaml/dune#7724, @emillon)

- Add a new alias `@doc-json` to build odoc documentation in JSON format. This
  output can be consumed by external tools. (ocaml/dune#8178, @emillon)

- Modules that were declared in `(modules_without_implementation)`,
  `(private_modules)` or `(virtual_modules)` but not declared in `(modules)`
  will raise an error. (ocaml/dune#7674, @Alizter)

- No longer emit linkopts(javascript) in META files (ocaml/dune#8168, @hhugo)

- Deprecate install destination paths beginning with ".." to prevent packages
  escaping their designated installation directories. (ocaml/dune#8350, @gridbugs)

- RPC message styles are now serialised meaning that RPC diagnostics keep their
  Ansi styling. (ocaml/dune#8516, fixes ocaml/dune#6921, @Alizter)

- Truncate output from actions that produce too much output (@tov, ocaml/dune#8351)

- Allow libraries to shadow OCaml builtin libraries. Previously, builtin
  libraries would always take precedence. (@rgrinberg, ocaml/dune#8558)

- Remove warning against `.dune` files generated by pre dune 2.0 (ocaml/dune#8611,
  @rgrinberg)

- `dune utop` no longer links `utop` in "custom" mode, which should make this
  command considerably faster. (ocaml/dune#8631, fixes ocaml/dune#6894, @nojb)

- Ensure that package names in `dune-project` are valid opam package names.
  (ocaml/dune#8331, @emillon)

- init: check that module names are valid (ocaml/dune#8644, fixes ocaml/dune#8252, @emillon)

- dune init: parse `--public` as a public name (ocaml/dune#8603, fixes ocaml/dune#7108, @emillon)

- Stop signing source files with substitutions. Sign only binaries instead
  (ocaml/dune#8361, fixes ocaml/dune#8360, @anmonteiro)

- Remove versions 0.1 and 0.2 of the experimental ctypes extension. (ocaml/dune#8293,
  @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants