Bug 1800 - PermitUserEnvironment accepting pattern of allowed userenv variables
Summary: PermitUserEnvironment accepting pattern of allowed userenv variables
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: 5.5p1
Hardware: All All
: P2 enhancement
Assignee: Damien Miller
URL:
Keywords:
Depends on:
Blocks: V_7_8
  Show dependency treegraph
 
Reported: 2010-07-18 14:18 AEST by Daniel Allen
Modified: 2023-01-13 13:40 AEDT (History)
3 users (show)

See Also:


Attachments
diff for patching 5.5p1 and 5.4p1 (9.45 KB, patch)
2010-07-18 14:18 AEST, Daniel Allen
no flags Details | Diff
patch replacement: diff -u (6.96 KB, patch)
2010-07-20 01:03 AEST, Daniel Allen
no flags Details | Diff
userenv patch for 5.8p1 (6.79 KB, application/octet-stream)
2011-03-19 05:24 AEDT, Daniel Allen
no flags Details
patch for PermitUserEnvironment against 5.9p1 (7.03 KB, patch)
2011-10-14 05:37 AEDT, Daniel Allen
no flags Details | Diff
rewrite of patch to use match_pattern_list (8.06 KB, patch)
2011-12-03 08:56 AEDT, Daniel Allen
no flags Details | Diff
redo (4.89 KB, patch)
2017-07-07 14:59 AEST, Damien Miller
no flags Details | Diff
include documentation, make whitelist apply to key/cert authorized_keys options (5.78 KB, patch)
2017-07-07 15:21 AEST, Damien Miller
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Allen 2010-07-18 14:18:41 AEST
Created attachment 1901 [details]
diff for patching 5.5p1 and 5.4p1

"PermitUserEnvironment=Yes" security risks could be mitigated by allowing sshd to allow selected user-environment variables.  I have written a patch which allows sshd configuration to specify:

"PermitUserEnvironment=VAR" 

This passes user environment variables (from $USER/.ssh/environment and/or $USER/.ssh/authorized_keys) starting with VAR, ignoring all other environment variables not previously copied by sshd.

The default option for PermitUserEnvironment is unchanged; it still defaults to "No".

As a second effect, if PermitUserEnvironment is set to the default "No", but an "environment=" option is specified in authorized_keys, the key is no longer rejected with a "Bad options in file" error, but instead silently ignores the "environment=" option, which is similar to the behaviour of other options such as "permitopen=".
Comment 1 Damien Miller 2010-07-19 13:24:34 AEST
Sorry to be a pest, but could you please resubmit your patch in unified diff format ("diff -u"). You only need submit the 5.5p1 version.

Thanks,
Damien
Comment 2 Daniel Allen 2010-07-20 01:03:35 AEST
Created attachment 1903 [details]
patch replacement: diff -u
Comment 3 Daniel Allen 2010-07-20 01:05:01 AEST
(In reply to comment #1)
> unified diff format ("diff -u"). You only need submit the 5.5p1 version.

Oops, my bad! Resubmitted. -Daniel
Comment 4 Damien Miller 2011-01-24 12:30:50 AEDT
Retarget unclosed bugs from 5.7=>5.8
Comment 5 Daniel Allen 2011-03-19 05:24:09 AEDT
Created attachment 2017 [details]
userenv patch for 5.8p1

redid patch for openssh-5.8p1
Comment 6 Damien Miller 2011-09-06 10:34:19 AEST
Retarget unresolved bugs/features to 6.0 release
Comment 7 Damien Miller 2011-09-06 10:36:32 AEST
Retarget unresolved bugs/features to 6.0 release
Comment 8 Damien Miller 2011-09-06 10:39:07 AEST
Retarget unresolved bugs/features to 6.0 release

(try again - bugzilla's "change several" isn't)
Comment 9 Daniel Allen 2011-10-14 05:37:12 AEDT
Created attachment 2098 [details]
patch for PermitUserEnvironment against 5.9p1

Keeping up with new version numbers.
Comment 10 Damien Miller 2011-10-21 11:42:39 AEDT
The patch looks okay, but I'm a little reticent to add a method to control environment variables that doesn't look like any of the other ACL mechanisms that we use. Perhaps this should use match_pattern_list() (match.[ch]) to test environment variables when PermitUserEnvironment!=yes instead of a simple substring?
Comment 11 Daniel Allen 2011-10-29 12:50:20 AEDT
(In reply to comment #10)
> The patch looks okay, but I'm a little reticent to add a method to
> control environment variables that doesn't look like any of the other
> ACL mechanisms that we use. Perhaps this should use
> match_pattern_list() (match.[ch]) to test environment variables when
> PermitUserEnvironment!=yes instead of a simple substring?

Oh! match_pattern_list() sounds like a much more robust solution.
I'll see if I can code something up soon. I have two big deadlines in the next two weeks but I hope to have something to review soon.

Thanks, Daniel
Comment 12 Daniel Allen 2011-12-03 08:56:02 AEDT
Created attachment 2113 [details]
rewrite of patch to use match_pattern_list

New patch allows pattern lists for permitted user environment variables, including wildcards and negation. 

New format to match exactly one variable:

PermitUserEnvironment "REMOTEUSER=*"

To match any variables starting with LOG and XTERM variables with values matching vt*:

PermitUserEnvironment "LOGNAME=*,XTERM=vt*"
Comment 13 Daniel Allen 2011-12-05 11:34:07 AEDT
(In reply to comment #12) 

> To match any variables starting with LOG and XTERM variables with
> values matching vt*:

the last line of the example should read:

PermitUserEnvironment "LOG*,XTERM=vt*"

instead of:

> PermitUserEnvironment "LOGNAME=*,XTERM=vt*"
Comment 14 Daniel Allen 2012-02-07 03:10:44 AEDT
Hi Damien, don't suppose you've had time to look at this patch yet? It's working well for our campus, and I'd love to see this making it into v6.0. Thanks, -Daniel
Comment 15 Damien Miller 2012-02-24 10:34:27 AEDT
Retarget from 6.0 to 6.1
Comment 16 Damien Miller 2012-02-24 10:38:07 AEDT
Retarget 6.0 => 6.1
Comment 17 Damien Miller 2012-09-07 11:38:15 AEST
Retarget uncompleted bugs from 6.1 => 6.2
Comment 18 Damien Miller 2012-09-07 11:40:41 AEST
Retarget bugs from 6.1 => 6.2
Comment 19 Damien Miller 2013-03-08 10:23:59 AEDT
retarget to openssh-6.3
Comment 20 Damien Miller 2013-07-25 12:17:56 AEST
Retarget to openssh-6.4
Comment 21 Damien Miller 2013-07-25 12:20:51 AEST
Retarget 6.3 -> 6.4
Comment 22 Damien Miller 2014-02-06 10:18:05 AEDT
Retarget incomplete bugs / feature requests to 6.6 release
Comment 23 Damien Miller 2014-02-06 10:20:10 AEDT
Retarget incomplete bugs / feature requests to 6.6 release
Comment 24 Damien Miller 2014-04-12 14:49:12 AEST
Retarget to 6.7 release, since 6.6 was mostly bugfixing.
Comment 25 Damien Miller 2014-04-12 14:53:38 AEST
Remove from 6.6 tracking bug
Comment 26 Damien Miller 2014-08-30 04:38:25 AEST
Retarget incomplete bugs to 6.8 release.
Comment 27 Damien Miller 2014-08-30 04:40:14 AEST
These bugs are no longer targeted at the imminent 6.7 release
Comment 28 Damien Miller 2015-03-03 07:59:34 AEDT
OpenSSH 6.8 is approaching release and closed for major work. Retarget these bugs for the next release.
Comment 29 Damien Miller 2015-03-03 08:01:16 AEDT
Retarget to 6.9
Comment 30 Damien Miller 2015-08-11 22:59:20 AEST
Retarget pending bugs to openssh-7.1
Comment 31 Damien Miller 2016-02-26 14:44:26 AEDT
Retarget to openssh-7.3
Comment 32 Damien Miller 2016-02-26 14:47:28 AEDT
Retarget to openssh-7.3
Comment 33 Damien Miller 2016-07-22 14:10:50 AEST
retarget unfinished bugs to next release
Comment 34 Damien Miller 2016-07-22 14:14:43 AEST
retarget unfinished bugs to next release
Comment 35 Damien Miller 2016-07-22 14:15:45 AEST
retarget unfinished bugs to next release
Comment 36 Damien Miller 2016-07-22 14:17:14 AEST
retarget unfinished bugs to next release
Comment 37 Damien Miller 2016-12-16 14:31:09 AEDT
OpenSSH 7.4 release is closing; punt the bugs to 7.5
Comment 38 Damien Miller 2017-06-30 13:43:17 AEST
Move incomplete bugs to openssh-7.6 target since 7.5 shipped a while back.

To calibrate expectations, there's little chance all of these are going to make 7.6.
Comment 39 Damien Miller 2017-06-30 13:44:33 AEST
remove 7.5 target
Comment 40 Damien Miller 2017-07-07 14:59:39 AEST
Created attachment 3012 [details]
redo

This redoes the patch from scratch; I think this is considerably simpler.

It supports PermitUserEnvironment=(yes|no|pattern-list)
Comment 41 Damien Miller 2017-07-07 15:21:41 AEST
Created attachment 3013 [details]
include documentation, make whitelist apply to key/cert authorized_keys options
Comment 42 Damien Miller 2018-04-06 13:12:19 AEST
Move to OpenSSH 7.8 tracking bug
Comment 43 Damien Miller 2018-07-03 21:02:45 AEST
This has been committed and will be in the next release.

commit 95344c257412b51199ead18d54eaed5bafb75617 (HEAD -> master, origin/master, origin/HEAD)
Author: djm@openbsd.org <djm@openbsd.org>
Date:   Tue Jul 3 10:59:35 2018 +0000

    upstream: allow sshd_config PermitUserEnvironment to accept a

    pattern-list of whitelisted environment variable names in addition to yes|no.

    bz#1800, feedback and ok markus@

    OpenBSD-Commit-ID: 77dc2b468e0bf04b53f333434ba257008a1fdf24
Comment 44 Damien Miller 2021-04-23 14:58:10 AEST
closing resolved bugs as of 8.6p1 release