Make calls to getenv(3) safer when setuid. #7047

paulidale · 2018-08-23T22:30:57Z

Change all calls to getenv() inside libcrypto to use a new wrapper function that use secure_getenv() if available and an OPENSSL_issetugid() then getenv() if not.

paulidale · 2018-08-23T22:33:51Z

I left the getenv(3) calls alone in the apps and tests. Neither should be running setuid or similar.

dot-asm · 2018-08-24T08:10:21Z

I'd say bad idea in the suggested form. For two reasons. 1. Does it actually have to be non-discriminating? I mean would it actually be appropriate to ignore all environment variables? Well, it's not like I have ready answer to the question, but at least I think that it should be posed, and I don't see that it was. I mean it pretty much looks like "replace everything just because". 2. What about other systems? Why does it have to be not only Linux-specific, but even glibc-version-specific? I mean if we consider this a security issue, then there should be fall-back code that provide corresponding functionality on other systems. In other words even from this angle it looks pretty much like "just because"...

paulidale · 2018-08-24T08:47:23Z

It isn't Linux specific although it is Unix specific. With glibc the safe secure_getenv(3) call is used, otherwise there is a check for privileged execution before allowing getenv(3) which wasn't present previously.

Unprivileged execution is unchanged -- this change only impacts programs that are setuid or have otherwise raised capabilities. Applications that are run either as a user or as root have unchanged access to their environment.

I'm having difficulty thinking of a situation where a setuid program would rely on unfettered access the environment unless I'm trying to hack the system.

dot-asm · 2018-08-24T15:49:13Z

It isn't Linux specific although it is Unix specific.

I apparently missed extra if in non-glibc case. I suppose I was tipped off by subject, which is actually misleading. As this is not about using secure_getenv when available, but changing getenv's behaviour completely. So what's with that? I mean totally misleading subject. [BTW, is this a feature? This is reference to assertion made elsewhere that current definition of "feature" is too imprecise to be useful.] But back on topic...

I'm having difficulty thinking of a situation where a setuid program would rely on unfettered access the environment unless I'm trying to hack the system.

Yet it's not about reading variable per se but what effect do they have, is it? Consider locale for example, would you deny setuid program to read LANG? One can ask if all variables interpreted by OpenSSL lib are equally sensitive, but you just can't make such general assertion about all environment variables, and base merge request [with misleading subject] on it. I probably should clarify that I'm not saying that I know the answer to the implied question, only that it's not given that such generalization is warranted.

paulidale · 2018-08-24T23:16:50Z

Yeah, bad subject choice. For that I apologise. I'm way too good at picking bad names and titles.

I too am not sure if all of the getenv(3) calls need to be changed or if some are intrinsically safe. Using your example, I can't think how an attacker could change LANG to get an exploit. However, I've repeatedly seen similarly benign things turn out to be exploitable. I erred heavily on the side of caution here. I'll go back and revisit each of the calls and try to make a more discerning determination.

kroeckx · 2018-08-24T23:48:18Z

What at least needs to use this is everything related to filenames and paths.

dot-asm · 2018-08-26T15:38:39Z

crypto/conf/conf_mod.c

@@ -481,7 +481,7 @@ char *CONF_get1_default_config_file(void)
    int len;

    if (!OPENSSL_issetugid()) {
-        file = getenv("OPENSSL_CONF");
+        file = ossl_safe_getenv("OPENSSL_CONF");


See the dissonance? One line up? Now jumping to conf_api. If config file. i.e. one that is not appointed by OPENSSL_CONF, is considered trusted, would it be appropriate to deny environment references?

dot-asm · 2018-08-26T15:39:24Z

crypto/engine/eng_list.c

@@ -318,7 +318,7 @@ ENGINE *ENGINE_by_id(const char *id)
     */
    if (strcmp(id, "dynamic")) {
        if (OPENSSL_issetugid()
-                || (load_dir = getenv("OPENSSL_ENGINES")) == NULL)
+                || (load_dir = ossl_safe_getenv("OPENSSL_ENGINES")) == NULL)


See the dissonance? One line up?

paulidale · 2018-08-26T21:59:53Z

I'd gone back to getenv(3) for the override processor flags.

I'm unsure about the LEGACY_GOST_PKCS12 environment variable in p12_mutl.c Safe or not?
I'm more sure about the _CONF_get_string which reads the environment in two places -- we don't know what these would be used for.

bernd-edlinger · 2018-08-27T10:21:43Z

crypto/rand/randfile.c

@@ -264,9 +264,9 @@ const char *RAND_file_name(char *buf, size_t size)
 #else
    if (OPENSSL_issetugid() != 0) {


should this be removed ?

Yes, it is removable. I've done so.
The slight difference on the else branch (the extra getenv) phased me.

dot-asm · 2018-08-28T08:23:01Z

crypto/conf/conf_api.c

@@ -82,7 +83,7 @@ char *_CONF_get_string(const CONF *conf, const char *section,
            if (v != NULL)
                return v->value;
            if (strcmp(section, "ENV") == 0) {
-                p = getenv(name);
+                p = ossl_safe_getenv(name);


As already said, at this point config file is supposed to be "trusted". I.e. in setuid case it won't be one appointed by OPENSSL_CONF environment variable. Would it be appropriate to suppress this? Or in other words wouldn't it be appropriate to say that if system owner says "read environment variable" is trusted config, then it should be done unconditionally?

Even if the configuration file is trusted, the environment can't be.

dot-asm · 2018-08-28T08:26:55Z

crypto/rand/randfile.c

@@ -262,11 +262,9 @@ const char *RAND_file_name(char *buf, size_t size)
        }
    }
 #else
-    if (OPENSSL_issetugid() != 0) {
+    if ((s = ossl_safe_getenv("RANDFILE")) == NULL || *s == '\0') {


Problem here is that suggested ossl_safe_getenv is "tri-state", denied, no-variable, value, and it's impossible to tell first two apart. As result of suggested change no-RANDFILE-variable is rendered to be equivalent of "denied". No good...

I don't see what's the problem here.

Yeah, "no good" was too strong. Formally speaking the flow is different because of "tri-state" thing, but it's true that overall outcome is the same in this case. Modulo redundant call to set[gu]id in case ossl_safe_getenv('RANDFILE") returns NULL...

dot-asm · 2018-08-28T08:27:18Z

crypto/sm2/sm2_crypt.c

+    sz = 2 * ASN1_object_size(0, field_size + 1, V_ASN1_OCTET_STRING)
+         + ASN1_object_size(0, md_size, V_ASN1_INTEGER)
+         + ASN1_object_size(0, msg_len, V_ASN1_INTEGER);
+    *ct_size = ASN1_object_size(0, sz, V_ASN1_SEQUENCE);


Unrelated modification.

Oops, I was testing the code I pasted into another PR. I'll remove.

dot-asm · 2018-08-28T08:39:47Z

Overall I'd argue against using safe_getenv. Less Linux dependencies is better. Because our goal is wider than Linux. Trouble is that Linux is the only one that gets real and thorough exercise. And in such case it would be only appropriate that non-Linux-specific code paths are exercised on Linux. As for ossl_safe_getenv, I'm a bit torn. But I'm inclined to say that if there is need to secure more environment variable references, explicit OPENSSL_issetugid tests around the problematic references might be more appropriate. Because ossl_safe_getenv is "tri-state", which might be wrong in some circumstances. Which means that there would be calls to ossl_safe_getenv, but there also would be calls to OPENSSL_issetugid. In which case a unified approach would be more readable, because reader and developer would be excused from struggling with question which one to use when.

petrovr · 2018-08-28T19:11:14Z

Andy Polyakov wrote:

dot-asm commented on this pull request. > @@ -82,7 +83,7 @@ char *_CONF_get_string(const CONF *conf, const char *section, if (v != NULL) return v->value; if (strcmp(section, "ENV") == 0) { - p = getenv(name); + p = ossl_safe_getenv(name); As already said, at this point config file is supposed to be "trusted". I.e. in setuid case it won't be one appointed by OPENSSL_CONF environment variable. Would it be appropriate to suppress this? Or in other words wouldn't it be appropriate to say that if system owner says "read environment variable" is trusted config, then it should be done unconditionally?

I cannot see how is such case someone would could use environment variables. If configuration (even trusted) has at least one environment it has to be ignored at all. Otherwise code will produce unexpected failures. As result in setuid programs we cannot have "trusted" config - only default with documentation that environment variables are risky. Abnormal failures is reason to vote against whole idea for "safe" use of environment variables.

t8m · 2018-09-05T15:26:43Z

The problem with OPENSSL_issetugid is that there can be other ways how to elevate privileges than just setuid/setgid - particularly there are the capabilities which can be elevated on file with similar mechanism as setuid/setgid. Glibc secure_getenv handles that.

paulidale · 2018-09-05T23:04:41Z

An attempt is made to use capabilities in OPENSSL_issetugid so it is doing more than checking the real and effective user and group IDs (on Linux). Regardless, I'd prefer secure_getenv since our checking will likely become dated over time and is Linux specific whereas glibc might keep up better.

paulidale · 2018-09-06T03:15:14Z

Travis failures are not relevant.

Change all calls to getenv() inside libcrypto to use a new wrapper function that use secure_getenv() if available and an issetugid then getenv if not. CPU processor override flags are unchanged. Extra checks for OPENSSL_issetugid() have been removed in favour of the safe getenv.

to be squashed

paulidale · 2018-09-11T22:51:42Z

Does this need more discussion or can it be merged now that 1.1.1 is live?

kroeckx · 2018-09-21T22:24:59Z

I think this is ready to get merged?

Change all calls to getenv() inside libcrypto to use a new wrapper function that use secure_getenv() if available and an issetugid then getenv if not. CPU processor override flags are unchanged. Extra checks for OPENSSL_issetugid() have been removed in favour of the safe getenv. Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #7047)

Change all calls to getenv() inside libcrypto to use a new wrapper function that use secure_getenv() if available and an issetugid then getenv if not. CPU processor override flags are unchanged. Extra checks for OPENSSL_issetugid() have been removed in favour of the safe getenv. Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #7047) (cherry picked from commit 5c39a55)

paulidale · 2018-09-24T03:17:16Z

Merged to master, 1.1.1 and 1.1.0. 1.0.2 will be a separate PR.

Thanks.

Manual merge of openssl#7047 to 1.0.2-stable.

paulidale · 2018-09-24T04:29:31Z

#7300 is the back port to 1.0.2-stable.
#7299 went weird when I clicked something wrongly.

Manual merge of openssl#7047 to 1.0.2-stable.

Manual merge of #7047 to 1.0.2-stable. Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #7300)

paulidale changed the title ~~Use secure_getenv(3) when available.~~ Make calls to getenv(3) safer when setuid. Aug 24, 2018

dot-asm reviewed Aug 26, 2018

View reviewed changes

bernd-edlinger reviewed Aug 27, 2018

View reviewed changes

paulidale force-pushed the getenv branch from 2dabc30 to 10f9553 Compare August 27, 2018 22:55

dot-asm reviewed Aug 28, 2018

View reviewed changes

mattcaswell added this to the Assessed milestone Sep 3, 2018

paulidale force-pushed the getenv branch from 10f9553 to 5bb51cb Compare September 5, 2018 23:18

bernd-edlinger approved these changes Sep 6, 2018

View reviewed changes

paulidale added 2 commits September 12, 2018 08:50

Remove redundant getenv check.

e1860cc

to be squashed

paulidale force-pushed the getenv branch from 5bb51cb to e1860cc Compare September 11, 2018 22:51

kroeckx added approval: done This pull request has the required number of approvals branch: master Merge to master branch branch: 1.0.2 Merge to OpenSSL_1_0_2-stable branch labels Sep 21, 2018

kroeckx added 1.1.0 branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels Sep 21, 2018

paulidale closed this Sep 24, 2018

paulidale deleted the getenv branch September 24, 2018 03:17

paulidale mentioned this pull request Sep 24, 2018

Merge 1.0.2 setuid calls to getenv(3) safety. #7299

Merged

paulidale added a commit to paulidale/openssl that referenced this pull request Sep 24, 2018

Merge 1.0.2 setuid calls to getenv(3) safety.

6d6606a

Manual merge of openssl#7047 to 1.0.2-stable.

paulidale mentioned this pull request Sep 24, 2018

Merge 1.0.2 setuid calls to getenv(3) safety. #7300

Closed

paulidale added a commit to paulidale/openssl that referenced this pull request Sep 25, 2018

Merge 1.0.2 setuid calls to getenv(3) safety.

52c2f64

Manual merge of openssl#7047 to 1.0.2-stable.

levitte pushed a commit that referenced this pull request Sep 26, 2018

Merge 1.0.2 setuid calls to getenv(3) safety.

788d2fa

Manual merge of #7047 to 1.0.2-stable. Reviewed-by: Bernd Edlinger <bernd.edlinger@hotmail.de> (Merged from #7300)

mspncp modified the milestones: Assessed, 1.1.1a Oct 23, 2018

mlindner mentioned this pull request Dec 17, 2018

Many calls to getenv remain after the changes to use ossl_safe_getenv instead #7917

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make calls to getenv(3) safer when setuid. #7047

Make calls to getenv(3) safer when setuid. #7047

paulidale commented Aug 23, 2018

paulidale commented Aug 23, 2018

dot-asm commented Aug 24, 2018

paulidale commented Aug 24, 2018

dot-asm commented Aug 24, 2018

paulidale commented Aug 24, 2018

kroeckx commented Aug 24, 2018 via email

dot-asm Aug 26, 2018

dot-asm Aug 26, 2018

paulidale commented Aug 26, 2018

bernd-edlinger Aug 27, 2018

paulidale Aug 27, 2018

dot-asm Aug 28, 2018

paulidale Sep 5, 2018

dot-asm Aug 28, 2018

bernd-edlinger Aug 28, 2018

dot-asm Aug 28, 2018

dot-asm Aug 28, 2018

paulidale Aug 28, 2018

dot-asm commented Aug 28, 2018

petrovr commented Aug 28, 2018 via email

t8m commented Sep 5, 2018

paulidale commented Sep 5, 2018

paulidale commented Sep 6, 2018

paulidale commented Sep 11, 2018

kroeckx commented Sep 21, 2018

paulidale commented Sep 24, 2018

paulidale commented Sep 24, 2018

		@@ -264,9 +264,9 @@ const char RAND_file_name(char buf, size_t size)
		#else
		if (OPENSSL_issetugid() != 0) {

Make calls to getenv(3) safer when setuid. #7047

Make calls to getenv(3) safer when setuid. #7047

Conversation

paulidale commented Aug 23, 2018

paulidale commented Aug 23, 2018

dot-asm commented Aug 24, 2018

paulidale commented Aug 24, 2018

dot-asm commented Aug 24, 2018

paulidale commented Aug 24, 2018

kroeckx commented Aug 24, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulidale commented Aug 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm commented Aug 28, 2018

petrovr commented Aug 28, 2018 via email

t8m commented Sep 5, 2018

paulidale commented Sep 5, 2018

paulidale commented Sep 6, 2018

paulidale commented Sep 11, 2018

kroeckx commented Sep 21, 2018

paulidale commented Sep 24, 2018

paulidale commented Sep 24, 2018