feat: disable file:// urls when hardening enabled #24858

gwossum · 2024-03-29T22:35:42Z

Stacks and templates allow specifying file:// URLs. Add option to disable their use for people who don't require them.

Fix tests on Darwin / MacOS. Also fixes spelling issue.

davidby-influx

A first pass; I can do a deeper review another day. Mostly about file names in errors...

pkg/fs/special_linux.go

pkger/parser.go

davidby-influx

LGTM. One question, not blocking.

davidby-influx · 2024-04-15T19:13:12Z

pkger/parser.go

+	// limit how much we read into RAM
+	var size int
+	size64 := st.Size()
+	if size64 > limitReadFileMaxSize {


Do we want to add a && limitReadFileMaxSize > 0 in case it becomes a variable some day? Not blocking on merge, just a thought.

jdstrand

Thanks for the PR! Most of my in-line comments are observations, but there are a few places where it would be good to update comments (eg, remocal and zero size files in /proc).

I also did some manual testing (using the amd64 deb from https://app.circleci.com/pipelines/github/influxdata/influxdb/38891/workflows/ec8d697d-ee31-46a3-9f15-2f1238f2eb8b/jobs/363718/artifacts) for /api/v2/stacks:

$ cat ~/stuff/ok.json
[
   {
      "apiVersion": "influxdata.com/v2alpha1",
      "kind": "Label",
      "metadata": {
         "name": "label-1"
      },
      "spec": {
         "color": "#eee888",
         "description": "desc_1"
      }
   },
   {
      "apiVersion": "influxdata.com/v2alpha1",
      "kind": "Bucket",
      "metadata": {
         "name": "rucket-1"
      },
      "spec": {
         "associations": [
            {
               "kind": "Label",
               "name": "label-1"
            }
         ],
         "description": "desc_1",
         "retentionRules": [
            {
               "everySeconds": 10000,
               "type": "expire"
            }
         ]
      }
   }
]

# in one terminal, start a http server to serve this over http://
$ cd ~/stuff
$ python3 -m http.server

# via http://
$ curl --insecure -H "Authorization: Token $TOKEN" --request POST "$URL/api/v2/stacks" --data "{\"orgID\":\"$ORGID\", \"name\":\"some name\", \"description\": \"some desc\", \"urls\": [\"http://localhost:8000/ok.json\"]}"
{
	"id": "0ce72523377b4000",
	"orgID": "697959a536e728a2",
	"createdAt": "2024-04-17T10:05:13.181224325-05:00",
...
$ influx apply -o $ORG --stack-id 0ce72523377b4000
LABELS    +add | -remove | unchanged
+-----+---------------+------------------+---------------+---------+-------------+
| +/- | METADATA NAME |        ID        | RESOURCE NAME |  COLOR  | DESCRIPTION |
+-----+---------------+------------------+---------------+---------+-------------+
| +   | label-1       | 0000000000000000 | label-1       | #eee888 | desc_1      |
+-----+---------------+------------------+---------------+---------+-------------+
|                                                           TOTAL  |      1      |
+-----+---------------+------------------+---------------+---------+-------------+
...

$ influx stacks rm --stack-id 0ce72523377b4000

# now try with file://
$ curl --insecure -H "Authorization: Token $TOKEN" --request POST "$URL/api/v2/stacks" --data "{\"orgID\":\"$ORGID\", \"name\":\"some name\", \"description\": \"some desc\", \"urls\": [\"file:///home/ubuntu/stuff/ok.json\"]}"
{
	"id": "0ce725ef413b4000",
	"orgID": "697959a536e728a2",
	"createdAt": "2024-04-17T10:08:42.116599762-05:00",
...

$ influx apply -o $ORG --stack-id 0ce725ef413b4000
LABELS    +add | -remove | unchanged
+-----+---------------+------------------+---------------+---------+-------------+
| +/- | METADATA NAME |        ID        | RESOURCE NAME |  COLOR  | DESCRIPTION |
+-----+---------------+------------------+---------------+---------+-------------+
| +   | label-1       | 0000000000000000 | label-1       | #eee888 | desc_1      |
+-----+---------------+------------------+---------------+---------+-------------+
|                                                           TOTAL  |      1      |
+-----+---------------+------------------+---------------+---------+-------------+
...

$ influx stacks rm --stack-id 0ce725ef413b4000

Now try stacks with 'hardening-enabled = true':

# the curl is expected to pass
$ curl --insecure -H "Authorization: Token $TOKEN" --request POST "$URL/api/v2/stacks" --data "{\"orgID\":\"$ORGID\", \"name\":\"some name\", \"description\": \"some desc\", \"urls\": [\"file:///home/ubuntu/stuff/ok.json\"]}"
{
	"id": "0ce72690ee408000",
	"orgID": "697959a536e728a2",
	"createdAt": "2024-04-17T10:11:27.673418946-05:00",
...

# the apply is not
$ influx apply -o $ORG --stack-id 0ce72690ee408000
Error: failed to check template impact: 400 Bad Request: invalid URL scheme: invalid URL scheme
$ influx stacks rm --stack-id 0ce72690ee408000

Now try stacks with just 'template-file-urls-disabled = true':

# the curl is expected to pass
$ curl --insecure -H "Authorization: Token $TOKEN" --request POST "$URL/api/v2/stacks" --data "{\"orgID\":\"$ORGID\", \"name\":\"some name\", \"description\": \"some desc\", \"urls\": [\"file:///home/ubuntu/stuff/ok.json\"]}"
{
	"id": "0ce7271e9d86b000",
	"orgID": "697959a536e728a2",
	"createdAt": "2024-04-17T10:11:27.673418946-05:00",
...

# the apply is not
$ influx apply -o $ORG --stack-id 0ce7271e9d86b000
Error: failed to check template impact: 400 Bad Request: invalid URL scheme: invalid URL scheme
$ influx stacks rm --stack-id 0ce7271e9d86b000

Now try /api/v2/templates/apply directly with neither hardening-enabled nor template-file-urls-disabled set:

# test http:// still works with /api/v2/templates/apply
$ curl --insecure -H "Authorization: Token $TOKEN" -H "Content-Type: application/json" --request POST "$URL/api/v2/templates/apply" --data "{\"orgID\": \"$ORGID\", \"dryRun\": true, \"remotes\": [{\"url\": \"http://localhost:8000/ok.json\"}]}"
{
	"sources": [
		"http://localhost:8000/ok.json"
	],
	"stackID": "",
...

# influx apply --encoding json -f works too  # the -f path is on the client, not server
$ influx apply --encoding json -f ~/stuff/ok.json
LABELS    +add | -remove | unchanged
+-----+---------------+------------------+---------------+---------+-------------+
| +/- | METADATA NAME |        ID        | RESOURCE NAME |  COLOR  | DESCRIPTION |
+-----+---------------+------------------+---------------+---------+-------------+
| +   | label-1       | 0000000000000000 | label-1       | #eee888 | desc_1      |
+-----+---------------+------------------+---------------+---------+-------------+
|                                                           TOTAL  |      1      |
+-----+---------------+------------------+---------------+---------+-------------+
...

# now try with file:///home/ubuntu/stuff/ok.json 
# Curiously, I was thinking should've been allowed since neither hardening-enabled nor
# template-file-urls-disabled was specified in the config file
$ curl --insecure -H "Authorization: Token $TOKEN" -H "Content-Type: application/json" --request POST "$URL/api/v2/templates/apply" --data "{\"orgID\": \"$ORGID\", \"dryRun\": true, \"remotes\": [{\"url\": \"file:///home/ubuntu/stuff/ok.json\"}]}"
{
	"code": "unprocessable entity",
	"message": "template from url[file:///home/ubuntu/stuff/ok.json] had an issue: Get \"file:///home/ubuntu/stuff/ok.json\": unsupported protocol scheme \"file\""

I then tried with the official 2.7.6-1 amd64 deb, and it also fails to process file:///home/ubuntu/stuff/ok.json with /api/v2/templates/apply. I then tried 2.1.1 (which predates #23030, which I though might've changed the behavior) and it fails in the same way.

So, /api/v2/stacks is working as designed by the PR. /api/v2/templates/apply unconditionally denies use of file:/// URLs and has for years. The difference in behavior is odd and makes me want to say that we should unconditionally disallow file:/// URLs in both for consistency, but we know that file:/// URLs are used with stacks, so I don't think we can do that.

Long story short, this PR is functionally correct.

cmd/influxd/launcher/cmd.go

pkg/fs/special_linux.go

jdstrand · 2024-04-17T14:10:33Z

pkg/fs/special_linux.go

+		return true, nil
+	}
+	// It's a special file unless the device ID matches /tmp's ID.
+	return devId != tmpDevId, nil


I think there will be problems with this implementation if the user specifies a file in /dev/shm or if https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#TemporaryFileSystem= is used and the user specifies something in there. The latter is rather esoteric and unlikely but the former seems plausible. I'm not sure if we should try to account for /dev/shm (note that some systems mount /dev/shm with a symlink from /run/shm to /dev/shm while other (older) systems do the opposite) or just mention it in a release note. A release note is probably fine.

IMHO, /dev/shm is only for POSIX shard memory, and it's not specifically in FHS. /run is definitely interesting, though, because it is almost always tmpfs and is guaranteed by FHS. It's also what systemd says you should use instead of /dev/shm. I'm thinking we should have an additional special case check for /run. We could also add a check against /dev/shm if you want.

I know some people have gotten /dev/shm (or /run/shm) under their fingers for having a reliably memory-backed sticky dir (/tmp-style) on Linux (eg, to generate private keys to keep off disk). That said, it feels unlikely that someone would put a template in /dev/shm (or /run/shm on a server) to then start running stacks command for influxdb. In thinking about this more, at most this is a release note IMHO.

As for /run, it's not impossible to imagine that people might put things under /run/user or /run/something. influxdb doesn't have a /run/influxdb directory with 2.x and /run/user is going to be challenging to use for users since it is only accessible to the user (and not the influxdb user) so I don't think this is required.

pkger/parser.go

pkg/fs/special_linux.go

pkger/service.go

Improve special file system exception checking for tmpfs on Linux to cover most common tmpfs mounts. Improve help for "--hardening-enabled" command line option. Improve some comments.

jdstrand

This is probably fine as is, but I have a small pedantic suggestion for /dev/shm and a few comment update suggestions. I'm not sure if the tests are flaky (I didn't check), but fyi, not all the tests are passing.

jdstrand · 2024-04-29T14:50:39Z

pkg/fs/special_linux.go

+	// the device ID won't match so we won't grant a tmpfs exception based on it.
+	// On Linux, every tmpfs mount has a different device ID, so we need to check
+	// against all common ones that might be in use.
+	tmpfsMounts := []string{"/tmp", "/run", "/dev/shm"}


/dev/shm is a symlink to /run/shm on old Linux's (eg, Debian Stretch, but there are others). Ubuntu 16.04 and later is known to have /dev/shm as the mountpoint (not a symlink). I did find reports of WSL in 2022 that had /dev/shm as a symlink. I think it would be good to handle that case too. AIUI, the old systems simply symlinked /dev/shm to point to the shm directory under the /run tmpfs (ie, /run/shm wasn't its own tmpfs). I think simply conditionally adding "/dev/shm" to tmpfsMounts if it is not a symlink is all that is needed here.

The good news is that the check for the special filesystem and tmpfs exceptions is against the target of the symlink, so the same code should work for that situation. I've added test cases with symlinks as well.

jdstrand · 2024-04-29T14:58:03Z

pkg/fs/special_linux.go

+		fDevId, err := getDevId(fSt)
+		if err != nil {
+			// See above for why we're returning an error here.
+			return math.MaxUint64, nil


The comment says "See above for why we're returning an error here" but we're return nil for error which is a little confusing for the reader. Can you rephrase the comment for clarity? (Not to mention, it seems some of the supporting comments for this are also 'below', not just 'above').

jdstrand · 2024-04-29T14:58:50Z

pkg/fs/special_linux.go

+	}
+	for _, fn := range tmpfsMounts {
+		// We ignore errors if getFileDevId fails, which could
+		// mean that the file (e.g. /run) doesn't exist. The error


This comment ends with a sentence fragment.

jdstrand · 2024-04-29T14:59:21Z

pkg/fs/special_linux.go

-		// We got an error getting stats on /tmp, but that just means we can't
-		// say the file is in tmpfs. We'll still go ahead and call it a special file.
-		return true, nil
+	// for each common tmpfs mount point, If the mount point's device ID matches this st's,


s/point,/point./ ?

gwossum added 2 commits March 29, 2024 15:37

feat: disable file:// urls when hardening enabled

00e2ff3

Stacks and templates allow specifying file:// URLs. Add option to disable their use for people who don't require them.

chore: update test

9ad9350

davidby-influx assigned gwossum Mar 29, 2024

davidby-influx self-requested a review March 29, 2024 23:02

davidby-influx added the area/2.x OSS 2.0 related issues and PRs label Mar 29, 2024

gwossum added 8 commits April 2, 2024 14:44

fix: add special file exception for tmpfs

81fd82e

chore: fix tests on darwin

711beb9

Fix tests on Darwin / MacOS. Also fixes spelling issue.

chore: modify tests for Windows

6a61b15

chore: fix static style checks

8d03376

chore: fix windows test cases

04fd6d8

chore: fix Windows test case

1dba47c

Merge branch 'main-2.x' into gw_edge_455

bc681cd

chore: fix formatting

cee11d8

gwossum marked this pull request as ready for review April 11, 2024 15:18

davidby-influx requested changes Apr 11, 2024

View reviewed changes

chore: improved error messages

b56f4b9

gwossum requested a review from davidby-influx April 15, 2024 16:55

davidby-influx approved these changes Apr 15, 2024

View reviewed changes

jdstrand requested changes Apr 17, 2024

View reviewed changes

gwossum added 2 commits April 26, 2024 17:54

fix: improve special file system exception checking for tmpfs

27af354

Improve special file system exception checking for tmpfs on Linux to cover most common tmpfs mounts. Improve help for "--hardening-enabled" command line option. Improve some comments.

chore: fix go vet warnings

8a9f143

gwossum requested review from davidby-influx and jdstrand April 29, 2024 03:57

jdstrand requested changes Apr 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: disable file:// urls when hardening enabled #24858

feat: disable file:// urls when hardening enabled #24858

gwossum commented Mar 29, 2024

davidby-influx left a comment

davidby-influx left a comment

davidby-influx Apr 15, 2024

jdstrand left a comment

jdstrand Apr 17, 2024

gwossum Apr 17, 2024

jdstrand Apr 18, 2024 •

edited

jdstrand left a comment

jdstrand Apr 29, 2024

gwossum May 2, 2024

jdstrand Apr 29, 2024

jdstrand Apr 29, 2024

jdstrand Apr 29, 2024

feat: disable file:// urls when hardening enabled #24858

Are you sure you want to change the base?

feat: disable file:// urls when hardening enabled #24858

Conversation

gwossum commented Mar 29, 2024

davidby-influx left a comment

Choose a reason for hiding this comment

davidby-influx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdstrand Apr 18, 2024 • edited

Choose a reason for hiding this comment

jdstrand left a 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

jdstrand Apr 18, 2024 •

edited