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

[opensuse] Add FileMetadataCheck check. #600

Conversation

marxin
Copy link
Contributor

@marxin marxin commented Jan 12, 2021

Second check separated from #594.

@marxin marxin marked this pull request as draft January 12, 2021 13:55
@marxin marxin force-pushed the opensuse-security-add-FileMetadataCheck branch 3 times, most recently from 14fc918 to 0fa76e4 Compare January 19, 2021 10:00
@marxin marxin force-pushed the opensuse branch 2 times, most recently from cf15d15 to a7f460d Compare January 19, 2021 19:33
@marxin marxin force-pushed the opensuse-security-add-FileMetadataCheck branch from 0fa76e4 to ffbae91 Compare January 20, 2021 08:36
@marxin marxin force-pushed the opensuse-security-add-FileMetadataCheck branch 2 times, most recently from 6cea459 to b02a935 Compare February 3, 2021 10:03
@marxin
Copy link
Contributor Author

marxin commented Feb 3, 2021

May I please ping this @mgerstner?

@mgerstner
Copy link
Contributor

May I please ping this @mgerstner?

Sure ... I first wanted to add the tests for the other check but didn't find time yet. I can prioritize this review here instead.

@mgerstner
Copy link
Contributor

@marxin where can one get current rpmlint2 builds in OBS these days?

@marxin
Copy link
Contributor Author

marxin commented Feb 3, 2021

@mgerstner
Copy link
Contributor

Well this check does not do at all yet what we (security team) intended. This check only triggers when a whitelisting entry exists, but it needs to trigger for any packaged file that matches certain criteria.

So currently we have CheckDeviceFiles and CheckWorldWritable. For device files any file that is a character or block device needs to trigger the check. The check should then only pass if a matching whitelisting entry for a file exists. Otherwise an error with badness should be raised. For world writable files any file that has a world writeable bit set should be handled equally. But symlinks and device files should be excluded for that check.

Similar to the file digest check we need to tie whitelisting entries to package names.

@marxin marxin force-pushed the opensuse-security-add-FileMetadataCheck branch from b02a935 to ca87b64 Compare February 4, 2021 15:59
@marxin
Copy link
Contributor Author

marxin commented Feb 4, 2021

Well this check does not do at all yet what we (security team) intended. This check only triggers when a whitelisting entry exists, but it needs to trigger for any packaged file that matches certain criteria.

I see.

So currently we have CheckDeviceFiles and CheckWorldWritable. For device files any file that is a character or block device needs to trigger the check. The check should then only pass if a matching whitelisting entry for a file exists. Otherwise an error with badness should be raised. For world writable files any file that has a world writeable bit set should be handled equally. But symlinks and device files should be excluded for that check.

Can we do these 2 checks in one? I mean what about listing package files that are either a device (block or character) or have a writable file. For these, we should find a whitelisting entry that will list all the files and their metadata match?

Am I right that you expect a different set of flags of a file for each of the 2 checks you have?

					"/tmp/.ICE-unix": {
						"type": "d",
						"mode": "1777",
						"owner": "root:root"
					}
...
					"/var/lib/named/dev/random": {
						"type": "c",
						"mode": "0666",
						"dev": "1,8",
						"owner": "root:root"
					},

Similar to the file digest check we need to tie whitelisting entries to package names.

Yep, we pretty much know how to define a configuration for it.

@mgerstner
Copy link
Contributor

Can we do these 2 checks in one? I mean what about listing package files that are either a device (block or character) or have a writable file. For these, we should find a whitelisting entry that will list all the files and their metadata match?

I am not quite sure yet what you mean. Is your question whether we should keep a single whitelist for both, device files and world writable files? I don't think this is so important. Currently in rpmlint1 we have two differently configured checks and thus two separate whitelists.

I don't think it is easy to come up with a generic metadata checker that catches both cases. What I currently do in rpmlint-checks is already a bit unclean but I managed to reuse the same checker code with different initialization data for device restrictions and for world-writable restrictions.

It would be good to be able to easily reuse the code to e.g. also implement the setuid-root binary restrictions and so on. But I believe still each type of restriction needs to have its own checker instantiation.

Am I right that you expect a different set of flags of a file for each of the 2 checks you have?

					"/tmp/.ICE-unix": {
						"type": "d",
						"mode": "1777",
						"owner": "root:root"
					}
...
					"/var/lib/named/dev/random": {
						"type": "c",
						"mode": "0666",
						"dev": "1,8",
						"owner": "root:root"
					},

Well for the device file we naturally need the minor and major IDs otherwise the settings can be shared.

@marxin marxin force-pushed the opensuse-security-add-FileMetadataCheck branch from ca87b64 to 2ce5a64 Compare February 10, 2021 08:19
@marxin
Copy link
Contributor Author

marxin commented Feb 10, 2021

All right, I implemented the 2 checks. It's tested very lightly, feel free to add more tests, please.
And please make a pull request with a description of the checks.

@marxin marxin marked this pull request as ready for review February 10, 2021 08:23
@lgtm-com
Copy link

lgtm-com bot commented Feb 10, 2021

This pull request introduces 1 alert when merging 2ce5a64 into 1f8b499 - view on LGTM.com

new alerts:

  • 1 for Unreachable code

@marxin marxin force-pushed the opensuse-security-add-FileMetadataCheck branch from 2ce5a64 to 56c7891 Compare February 10, 2021 11:59
Copy link
Contributor

@mgerstner mgerstner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device file check looks good. The world writable check requires some tuning but after that should also work out.

@mgerstner
Copy link
Contributor

With this fix the basic check should work as expected. I can then add more tests and the description in a follow-up PR.

@marxin
Copy link
Contributor Author

marxin commented Feb 18, 2021

Great, thanks.

@marxin marxin merged commit 557e437 into rpm-software-management:opensuse Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants