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

Monit: support quotes in passwords #7156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fbrendel
Copy link
Member

Passwords containing single quotes must be bounded by double quotes and vice versa.
So passwords with double quotes needs to be bounded by single quotes.
See #6748

@fichtner
Copy link
Member

And if it has both? ;)

@fbrendel
Copy link
Member Author

Then we could encode it.
123'"456 becomes '123\0x27\0x22456'
But wait, what if the password contains the sequence \0x22 ?

@MAkcanca
Copy link

Highly unlikely but could happen. However I think it's better than the case before, where we don't handle double quotes. I had rough couple of hours trying to figure out why my monit setup don't send anything out then I discovered my password contains some quotes and it broke monit config. However if I was informed or got prevented from saving that password in the web page setup form, none of this would've happened.
Can we modify the web form so it prevents certain stuff?

@fichtner
Copy link
Member

fichtner commented Jan 23, 2024

I want to be very honest: security "researchers" don't give a damn about probability and this is clearly problematic from a technical point of view. Touch this once and fix all the issues is the best approach or else you will end up spending more time and more time again and processing a CVE eventually...

That being said single and double quotes have different escaping rules. In the shell you can use different quotes to write problematic quotes. I don't know how this works for monit but I also don't want to investigate.

"' can be escaped as '"'"'" in the shell, but bison probably uses escapes here so it needs to be checked which escape sequences are less for which quote and pick that as a standard and only deal with the remaining offending quote char separately (and escape the escape character by default, so \ becomes \\).

Cheers,
Franco

@fichtner fichtner self-assigned this Jan 23, 2024
@MAkcanca
Copy link

@fichtner But monit doesn't use shell to parse? Maybe I'm missing the point.
See
https://github.com/arnaudsj/monit/blob/aa491f6ca2d2dd79abebf1e97b6f3a14fd4453e8/util.c#L396
https://github.com/arnaudsj/monit/blob/aa491f6ca2d2dd79abebf1e97b6f3a14fd4453e8/l.l#L408

But if you can make it work, that would be awesome. Good luck!

@fichtner
Copy link
Member

I'm only explaining my point because the approach so far is not cutting it.

@fbrendel
Copy link
Member Author

fbrendel commented Jan 24, 2024

Ok, then, to be on the safe site, we have to encode all charachters.
@fichtner is it possible to do that via a new field type e.g. HexField?
The idea is to store the whole password as hex string which is compatible with the Monit syntax.

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

Successfully merging this pull request may close these issues.

None yet

3 participants