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

curl tool: erase some more sensitive command line arguments #7964

Closed
wants to merge 1 commit into from

Conversation

@monnerat
Copy link
Contributor

@monnerat monnerat commented Nov 5, 2021

As the ps command may reveal sensitive command line info, obfuscate
options --tlsuser, --tlspasswd, --proxy-tlsuser, --proxy-tlspassword and
--oauth2-bearer arguments.

src/tool_getparam.c Outdated Show resolved Hide resolved
src/tool_getparam.c Show resolved Hide resolved
@monnerat monnerat force-pushed the obfuscate_args branch from a64fd30 to 29cb514 Nov 5, 2021
@dwmw2
Copy link
Contributor

@dwmw2 dwmw2 commented Nov 5, 2021

This seems wrong, because it encourages people to put such things on the command line where they can be seen during process startup, up to the point where they get obfuscated. It just turns it into a race condition. Better never to obfuscate it; pass it in on file descriptors or some other way.

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Nov 5, 2021

By that logic we should also not support any plaintext protocols etc. I don't think we should let perfect be the enemy of good.

@dwmw2
Copy link
Contributor

@dwmw2 dwmw2 commented Nov 5, 2021

By that logic we should also not support any plaintext protocols etc. I don't think we should let perfect be the enemy of good.

That seems very different to me. Plaintext protocols have their place, and a user can deliberately choose to use them. Although perhaps they should be disabled (or auth with them should be disabled) by default to avoid being a trap for the unwary?

But this is different. If someone can be snooping on your command lines, it's never acceptable to put sensitive information on the command lines. Erasing it later really is just setting a trap for the naïve user, to make it look like there's no problem, when in fact there is.

@jay
Copy link
Member

@jay jay commented Nov 5, 2021

Though we're already doing this for several options I don't like it. There is a period of time where the arguments are visible so I don't see how it provides security. I think a fix is not our responsibility. hidepid can be set in some more recent OSes.

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Nov 6, 2021

This seems wrong, because it encourages people to put such things on the command line where they can be seen during process startup

and

Though we're already doing this for several options I don't like it.

What you both don't like is the ability to have such command line options: this is not the target of this PR.
As curl does support them, these options should be as smart as possible in the confidentiality direction, even if not perfect.
This PR only completes what is already done in the curl tool for some other options and does not implement a new feature.

IMHO, wether curl should support such options or not is out of this PR's scope.

There is a section about it in Everything curl: https://everything.curl.dev/cmdline/passwords#command-line-leakage
The curl.1 man page mentions this for --user and --proxy-user options: if something has to be added to the current PR, this would be similar doc notes for all options obfuscating their arguments.

@jay
Copy link
Member

@jay jay commented Nov 6, 2021

What you both don't like is the ability to have such command line options

Not at all, I find them quite convenient. I just don't think erasing them from the command line improves security if they can be accessed beforehand.

@jzakrzewski
Copy link
Contributor

@jzakrzewski jzakrzewski commented Nov 7, 2021

I just don't think erasing them from the command line improves security if they can be accessed beforehand.

Yeah, it just gives you a false sense of security. I have machines where I don't have to care about it at all and machines that I could get in trouble by ever putting sensitive information on the command line. The CLI does get saved in the history file after all by default.

@vszakats
Copy link
Member

@vszakats vszakats commented Nov 11, 2021

While extending this feature to more options seems sensible for consistency, I side with those saying this isn't an ideal solution in general. It's also a feature that may or may not be supported or detected (and thus enabled) on certain platforms/compilers and certain builds, so in practice the caller cannot count on it being available. (And even if available, it will only shorten the visibility of the secret as already said before.)

--config /dev/stdin, --netrc-file /dev/stdin, --form mysecret=@-, --data-urlencode mysecret@-, --header @/dev/stdin and similar/variations are more robust alternatives.

This can be tricky sometimes, so addressing it with documentation would probably be helpful.

@monnerat monnerat force-pushed the obfuscate_args branch 2 times, most recently from d701a8d to edfb83a Nov 27, 2021
@infinnovation-dev
Copy link

@infinnovation-dev infinnovation-dev commented Nov 29, 2021

Linguistic note: "sensible" is a faux ami - to a native English speaker it generally means "not foolish". I'd suggest using "sensitive".

@monnerat monnerat changed the title curl tool: erase some sensible command line arguments curl tool: erase some more sensitive command line arguments Nov 29, 2021
@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Nov 29, 2021

Linguistic note: "sensible" is a faux ami

Thanks for the remark: I was hesitating.

@monnerat monnerat force-pushed the obfuscate_args branch 2 times, most recently from 0bce280 to 7a4d64e Dec 6, 2021
@monnerat monnerat force-pushed the obfuscate_args branch 4 times, most recently from 219a106 to 0aa81a9 Dec 15, 2021
@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Dec 16, 2021

This is not an ideal solution, as has been discussed above, but we have these options and users will continue to use them regardless of what we think of passing sensitive information on the command line. So the question becomes, which option is safer?

  1. Removing the options as fast as we can when we can, and documenting caveats as well as the importance of not using command line for sensitive data
  2. Only documenting that the command line can be unsafe for sensitive data

My pessimistic take is that not enough users will read the documentation so I'm in favor of option 1.

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Dec 16, 2021

My pessimistic take is that not enough users will read the documentation so I'm in favor of option 1.

First, the PR is about consistency with what we have now, independently of considering having these options is good or not. Applying it will not make curl worse.

Next, although sharing your opinion about the majority of users, I think it's not reasonable to forget the advised minority and simply suppress these options without having a convenient alternate way of providing them. In addition, there are plenty of legitimate uses of them where confidentiality does not matter.

This discussion should ideally take place elsewhere and, if the PR is rejected, sensitive options blanking should logically be removed completely.

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Dec 16, 2021

First, the PR is about consistency with what we have now, independently of considering having these options is good or not. Applying it will not make curl worse.

Agreed.

Next, although sharing your opinion about the majority of users, I think it's not reasonable to forget the advised minority and simply suppress these options without having a convenient alternate way of providing them. In addition, there are plenty of legitimate uses of them where confidentiality does not matter.

Yeah, I don't think anyone is arguing for removing these.

... if the PR is rejected, sensitive options blanking should logically be removed completely.

Agreed as well.

@bagder
Copy link
Member

@bagder bagder commented Dec 16, 2021

These are all good arguments and I'm agreeing. Clearing the options as we do now, and thus accepting this PR, seems to be the least bad option even if it is far from perfect. We should warn users of the risk of using sensitive data in command lines.

@vszakats
Copy link
Member

@vszakats vszakats commented Dec 16, 2021

One problem with removing command-line options that are involved with secrets is that practically all --data*, --form* and --header options and even the URL (--url) argument can be used to pass around secrets. Many APIs will allow or often require secrets there. So maybe this falls into a similar-ish territory as --insecure/-k/--proxy-insecure/--doh-insecure and there should be some way to detect/warn and/or prevent these uses.

@monnerat: While you are at this, -E, --cert <certificate[:password]> and --proxy-cert <cert[:passwd]> options also accept secrets, so maybe it'd be nice to include them in this patch.

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Dec 16, 2021

While you are at this, -E, --cert <certificate[:password]> and --proxy-cert <cert[:passwd]> options also accept secrets, so maybe it'd be nice to include them in this patch.

This is already done in GetFileAndPassword().

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Dec 16, 2021

One problem with removing command-line options that are involved with secrets is that practically all --data*, --form* and --header options and even the URL (--url) argument can be used to pass around secrets. Many APIs will allow or often require secrets there. So maybe this falls into a similar-ish territory as --insecure/-k/--proxy-insecure/--doh-insecure and there should be some way to detect/warn and/or prevent these uses.

Maybe we could have an environment variable like CURL_NO_INSECURE_PARAMS (or similarly worded) which when set, curl will refuse to run when one of the above parameters are used (with an error explaining why). Kind of like how we've discussed to have a variable to prevent clear-text protocols. Not that it would move the needle that much, but it could be another tool for users to make informed decisions.

@vszakats
Copy link
Member

@vszakats vszakats commented Dec 16, 2021

@monnerat: Ops, I'm sorry, I didn't look there. All is fine then!

@vszakats
Copy link
Member

@vszakats vszakats commented Dec 16, 2021

@danielgustafsson: A envvar would be useful indeed. But, since an envvar can be overridden by apps, I was also having some other method in mind like placing a file with certain content at a non-writable (by the app) path (this can be /etc/ or %WINDIR% 1).

Footnotes

  1. Where instead of the WINDIR envvar, the appropriate API should be used to determine it.

@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Dec 16, 2021

involved with secrets is that practically all --data*, --form* and --header options and even the URL

Sure! We are blanking what surely contains sensitive data. I don't think we can blank everything.

We should warn users of the risk of using sensitive data in command lines.

With regard to the above, where to warn? Options doc pages seem not the good place as a majority of options may be concerned.

@dwmw2
Copy link
Contributor

@dwmw2 dwmw2 commented Dec 16, 2021

Maybe we could have an environment variable like CURL_NO_INSECURE_PARAMS (or similarly worded)

Surely it should be the other way round or it's pointless.
CURL_ALLOW_INSECURE,_COMMAND_LINE_PARAMETERS_AND_I_KNOW_THAT_BLANKING_THEM_AFTER_THE_ATTACKER_ALREADY_SAW_THEM_IS_POINTLESS

@vszakats
Copy link
Member

@vszakats vszakats commented Dec 16, 2021

<brainstorm>
Having --insecure/etc in mind libcurl/curl could dump warnings in a pre-configured log file (in "warn" mode), and in "enforcement" mode, the insecure options could also be overridden with a secure setting. For secrets-over-the-command-line, only a "warn" mode seems to make sense and the event could similarly be logged to a log file.

Such log file could be configured via an envvar or via a disk file at a predefined read-only location.
</brainstorm>

(This aims to cover the use-case where I'm as a user want to verify 3rd-party software using curl/libcurl for insecure practices, and possibly steer them towards secure behaviour. Without touching the source code or binary.)

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Dec 16, 2021

Surely it should be the other way round or it's pointless.

No. This isn't so much about helping/aiding those who a) know what they're doing or b) don't care. They won't use this anyways. This is helpful to set in order to catch scripts and applications which run curl under the hood with offending parameters. Just like how the environment for textual protocols are to catch apps using libcurl.

@jay
Copy link
Member

@jay jay commented Dec 16, 2021

(On environment variables only, and not an objection to this PR)

I'm against adding those types of environment variables that may break existing scripts. Assorted builds of curl can be used by many different applications (perhaps this is more common in Windows?) and if an admin sets CURL_NO_INSECURE or something then scripts using curl could break. To a lesser extent I did not like CURL_SSL_BACKEND for that reason (though otherwise the implementation is solid). Users need to know their scripts are going to function as intended and environment variables complicate that. I think most people are not interested in what we do here they just want something that works. They don't care that xyz is exposed, they don't care that --insecure is used, they don't care, just work. I really don't know that we should be saving users from themselves, because frankly I don't think they want saving. I think of it like a car that automatically puts your seatbelt on (there were actually cars that did that when I was younger) vs one that doesn't. What is next, we have to put curl --no-seatbelts in all our scripts?

edit: For the record, yes I do wear a seatbelt :)

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Dec 16, 2021

I'm against adding those types of environment variables that may break existing scripts ..

For the record I totally get where you're coming from, and I don't disagree. I still think these environment vars are a good idea since they IMHO aren't targeting the user you have in mind, and won't be used by them. The way I see it they are targeting power users (like you and me) who would like to impose restrictions on third-party usage of curl/libcurl. So in other words, they are strictly opt-in and won't wreak havoc on random peoples daily life.

I think of it like a car that automatically puts your seatbelt on (there were actually cars that did that when I was younger)

Yes! I remember my cousin in Boston having a car like that in the early 90's to get cheaper insurance, those things were crazy. Oh my, the memories..

edit: For the record, yes I do wear a seatbelt :)

You are a clever man, stay safe!

@bch
Copy link
Contributor

@bch bch commented Dec 16, 2021

@vszakats
Copy link
Member

@vszakats vszakats commented Dec 16, 2021

Any such envvar or method will not be used by somebody who isn't interested in looking deeply into script/app behaviour. Some ("power users") may prefer an app to break rather than work via an insecure connection behind the scenes. Or in "warn" mode the app would not even break, but would let interested users detect the issue and report it to app developers (or look for alternative software.)

At the moment there is no way to tell what a curl/libcurl-using app is doing without inspecting the source code, which is most of time impractical and more often impossible. You can close port 80 via the firewall, but there is hardly any automated way to detect --insecure, without special tools and targeted testing.

So, it's not about saving anybody from themselves, but rather about saving yourself from 3rd party code using insecure practices. Staying with @jay's analogy, it's a tool for you (the driver) to see if the airbag installed in your car is actually connected and does work as expected.

(Having said that, such feature may also be useful for testing own code (for devs who are interested), though for such case static/compile-time methods may be work even better, where feasible.)

@bagder
Copy link
Member

@bagder bagder commented Dec 20, 2021

I think any such environment variable idea is outside the scope of this particular PR.

The most extreme case of this PR would of course be to blank all command line options always since almost everyone can leak information. Then we run into the problem that all curl processes will look identical in a process listing, with no good means to differentiate them - like if one or another takes too long time and you want to kill it... I can't say that I think that is a good idea.

@danielgustafsson
Copy link
Member

@danielgustafsson danielgustafsson commented Dec 20, 2021

As the ps command may reveal sensitive command line info, obfuscate
options --tlsuser, --tlspasswd, --proxy-tlsuser, --proxy-tlspassword and
--oauth2-bearer arguments.

Reported-by: Stephen Boost <s.booth@epcc.ed.ac.uk>
bagder
bagder approved these changes Jan 10, 2022
@bagder
Copy link
Member

@bagder bagder commented Jan 10, 2022

Thanks!

@bagder bagder closed this in b6acbde Jan 10, 2022
@monnerat
Copy link
Contributor Author

@monnerat monnerat commented Jan 10, 2022

Thanks for pulling!

@monnerat monnerat deleted the obfuscate_args branch Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants