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

640 triaging issue #642

Open
wants to merge 2 commits into
base: rewrite_for_6.0
Choose a base branch
from
Open

Conversation

alessandro-fazzi
Copy link
Member

This PR is just a triage for @castilma 's issue #640 . The CI will run only dedicated test.

This is not meant to be merged.

@alessandro-fazzi alessandro-fazzi changed the base branch from master to rewrite_for_6.0 February 12, 2022 14:15
@alessandro-fazzi alessandro-fazzi changed the title 640 triageing issue 640 triaging issue Feb 12, 2022

context 'testing @castilma\'s big report' do
let(:command) do
'mysqldump --host=remote_database_host --user=user ' \

Choose a reason for hiding this comment

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

Where is the code that creates this commandline normally? The escaping may happen when the commandline is constructed.

Choose a reason for hiding this comment

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

Disclaimer: I'm not a ruby programmer. I may say things that are wrong, because I misunderstood the code.

found it:here
(the same is happening a few lines down and here)

So there does happen shellescaping of the password for the commandline, not just for the regex to filter secrets.
So the regex used for substituting secrets should search for shellescaped secrects!

Another potential issue:
Shellwords.escape() escapes for the bourne shell, but you are using system(), which calls whatever the users current shell is. So it might break for unusual shells.

Additionally, system() is difficult to use safely and correctly, when the passed argument is constructed dynamically from user input.
I recommend using fork() followed by exec(), in the second or third form in the documentation, i.e. pass an array of strings.
That way you don't need to worry about string escaping! You just pass an array of the used arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

Disclaimer: I'm not a ruby programmer. I may say things that are wrong, because I misunderstood the code.

No problem; you'd be welcome anyway :)

found it:here (the same is happening a few lines down and here)

So there does happen shellescaping of the password for the commandline, not just for the regex to filter secrets. So the regex used for substituting secrets should search for shellescaped secrects!

This seems a good catch to me! But it seems more an endemic problem the fact we're logging escaped commands more than thinking about searching for escaped secrets. I have to tinker a bit on this.

Another potential issue: Shellwords.escape() escapes for the bourne shell, but you are using system(), which calls whatever the users current shell is. So it might break for unusual shells.

Why do you say system will call «whatever the user's current shell is»? In my knowledge system calls, in any form, should use the standard shell; which means, in *nix systems, /bin/sh. Would you mind to point me where you found that information? TIA

Additionally, system() is difficult to use safely and correctly, when the passed argument is constructed dynamically from user input. I recommend using fork() followed by exec(), in the second or third form in the documentation, i.e. pass an array of strings. That way you don't need to worry about string escaping! You just pass an array of the used arguments.

If your point is that it's difficult to use system due to the occurring shell expansion, keep in mind that the method has the same overloaded signature as exec. If that was not your point then I'm not catching it, sorry.

Instead I'm not prone to introduce threaded patterns (fork) because I've not enough knowledge to control them and sincerly I do not take what benefit it should bring on the table here. But I'm not interested in preventing a discussion about that if I am missing something you consider important :)


Thanks for the effort; I'll be on this as soon as I'll have a chunk of spare time

Choose a reason for hiding this comment

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

The system() documentation refers to exec(), which uses /bin/sh by default, but only after looking the environment variable RUBYSHELL.
(So I was wrong/unclear, but am right now :))

I did not notice that one could use system() without a subshell. So, one could just change to use system() with each argument separately passed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the patience and feedback.

I've never used that signature of #system, but my feeling is that it could be more functional and more straight to read/write.

I'll deepen on it a bit and will try to bring out something useful. For sure I won't target the 5.x version BTW.

Choose a reason for hiding this comment

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

If you want to do this in sometime, don't just forget it, because the stale bot closed the issue.

@stale
Copy link

stale bot commented Apr 17, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants