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

Duplicate entries in executables file lead to undefined behavior #5525

Closed
andreasabel opened this issue Aug 19, 2021 · 4 comments · Fixed by #6086
Closed

Duplicate entries in executables file lead to undefined behavior #5525

andreasabel opened this issue Aug 19, 2021 · 4 comments · Fixed by #6086
Assignees
Labels
exec About calls to executables from Agda tactics reflection Elaborator reflection, macros, tactic arguments security hazard Issue in Agda that could be exploited by a malicious actor type: bug Issues and pull requests about actual bugs
Milestone

Comments

@andreasabel
Copy link
Member

Discovered when looking at #5070. Ping @wenkokke.
Agda accepts silently executables with the same basename:

E.g. I can write this into .agda/executables:

/bin/echo
/Users/abel/foetus/sml/echo

(The second command does not echo, but crashes if called on the command line.)

This makes the example from the docs fail:

{-# OPTIONS --allow-exec #-}

open import Agda.Builtin.Equality
open import Agda.Builtin.List
open import Agda.Builtin.Reflection
open import Agda.Builtin.Reflection.External
open import Agda.Builtin.Sigma
open import Agda.Builtin.String
open import Agda.Builtin.Unit

_>>=_ = bindTC

macro
  echo : List String  Term  TC ⊤
  echo args hole = do
    (exitCode , (stdOut , stdErr))  execTC "echo" args ""
    unify hole (lit (string stdOut))

_ : echo ("hello""world" ∷ []) ≡ "hello world\n"
_ = refl

Error:

"" != "hello world\n" of type String
when checking that the expression refl has type
"" ≡ "hello world\n"

It succeeds if I swap the entries in the executables file.
This behaviour can be explained by a use Map.fromList, which takes the last of the duplicate entries and throws the others away silently. (You have been warned by @gallais!)

parseExecutablesFile ef files =
fmap (Map.fromList . catMaybes) . forM files $ \(ln, fp) -> do

This is a potential security problem!

Also, Agda swallows the OS error thrown by the second executable and simply returns the empty string. That's DOS/Windows-style error swallowing, not good...

Agda should report:

  • conflicting entries in executables
  • errors (or at least exitcodes) thrown by run executables.
@andreasabel andreasabel added type: bug Issues and pull requests about actual bugs reflection Elaborator reflection, macros, tactic arguments exec About calls to executables from Agda tactics security hazard Issue in Agda that could be exploited by a malicious actor labels Aug 19, 2021
@andreasabel andreasabel added this to the 2.6.3 milestone Aug 19, 2021
@wenkokke
Copy link
Contributor

Aiii, that shouldn’t be happening! I’ll draft a fix sometime next week!

@andreasabel
Copy link
Member Author

I’ll draft a fix sometime next week!

Such "famous last words" abound on github... 😆

@wenkokke : A simple fix would be fine at this point, without tackling #5553.

@wenkokke
Copy link
Contributor

@andreasabel: My apologies, I am still unable to type due to my wrist injury. Have been since May.

@andreasabel
Copy link
Member Author

@andreasabel: My apologies, I am still unable to type due to my wrist injury. Have been since May.

@wenkokke : Ah, did not know, sorry to hear.

Thanks for the quick feedback. I unassign you so that others can jump in. I wish a good recovery!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exec About calls to executables from Agda tactics reflection Elaborator reflection, macros, tactic arguments security hazard Issue in Agda that could be exploited by a malicious actor type: bug Issues and pull requests about actual bugs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants