-
Notifications
You must be signed in to change notification settings - Fork 56
Show port provenance and prevent source name/prefix spoofing #331
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -494,6 +494,15 @@ def load_sources(self): | |
| self.callback.message(" - {}".format(_("Loading Sources."))) | ||
|
|
||
| check_keys = {'version': None, 'prefix': None, 'api': HM_SOURCE_APIS, 'name': None, 'last_checked': None, 'data': None} | ||
|
|
||
| reserved_prefixes = {} | ||
| reserved_names = {} | ||
| for default_file, default_text in HM_SOURCE_DEFAULTS.items(): | ||
| default_data = json_safe_loads(default_text) | ||
| if default_data is not None: | ||
| reserved_prefixes[default_data['prefix']] = default_file | ||
| reserved_names[default_data['name']] = default_file | ||
|
|
||
| for source_file in source_files: | ||
| with source_file.open() as fh: | ||
| source_data = json_safe_load(fh) | ||
|
|
@@ -536,10 +545,35 @@ def load_sources(self): | |
| source_data['last_checked'] = None | ||
| source_data['data'] = {} | ||
|
|
||
| # A reserved prefix only loads from its bundled file, even if the | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a lot of extra code to protect against a security issue that's not really relevant. "Imposter" source files are fine if somebody wants to do that on their handheld.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're looking at user control, and I agree, however my approach has more to do with injection. Not that I would do this, but take Pharos for example. Pharos can already download my gmtoolkit.aarch64 binary and put it in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ??? we run ports at root, a bad actor can do whatever tf they want. Unless you can demonstrate a remote vuln without device or access to an installed port (and don't give me DNS poisoning like I mean something real) this is a dead end and I would vote against adding cruft for a non-issue. This feels to me like an LLM told you that this is a security vulnerability because the LLM doesn't understand the zero-security model we use for handheld devices where we run unaudited chinese firmwares and assume that devices are already rootkitted so our model is to just keep any sensitive info off-device (and frankly suggest partitioning your home network too).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is me recognizing JohnnyOnFlame's concerns, finding that when I use portmaster gui's infrastructure as advertised that my port is indistinguishable from a portmaster official port, and promptly fixing it because if I can do it, then so can anyone else. Ok not to add. Just pointing it out and attempting to contribute a solution. |
||
| # imposter file sorts first. | ||
| if reserved_prefixes.get(source_data['prefix'], source_file.name) != source_file.name: | ||
| logger.error(f"Reserved prefix {source_data['prefix']!r} in {source_file}, skipping.") | ||
| continue | ||
|
|
||
| # First file wins a prefix; a later source can't replace it. | ||
| if source_data['prefix'] in self.sources: | ||
| logger.error(f"Duplicate source prefix {source_data['prefix']!r} in {source_file}, skipping.") | ||
| continue | ||
|
|
||
| source = HM_SOURCE_APIS[source_data['api']](self, source_file, source_data) | ||
|
|
||
| self.sources[source_data['prefix']] = source | ||
|
|
||
| name_counts = {} | ||
| for source in self.sources.values(): | ||
| name_counts[source.name] = name_counts.get(source.name, 0) + 1 | ||
|
|
||
| for source in self.sources.values(): | ||
| file_name = source._file_name.name | ||
| if source.name in reserved_names: | ||
| if file_name == reserved_names[source.name]: | ||
| continue | ||
| elif name_counts[source.name] < 2: | ||
| continue | ||
|
|
||
| source.display_name = f"{source.name} ({file_name.rsplit('.source.json', 1)[0]})" | ||
|
|
||
|
|
||
| def _get_pm_signature(self, file_name): | ||
| """ | ||
|
|
@@ -1579,6 +1613,9 @@ def port_info(self, port_name, installed=False): | |
| else: | ||
| result = port_info_load(source.port_info(port_name)) | ||
|
|
||
| if result is not None and 'repo' not in result.get('source', {}): | ||
| result.setdefault('source', {})['repo'] = source.display_name | ||
|
|
||
| if result is None: | ||
| self.__PORT_INFO_CACHE[port_key] = result | ||
| return None | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why inject the source into the description? Wouldn't it be better to keep these two semantic things distinct and have the source displayed in the layout side of things instead of polluting description?
Ok, I read the submission wall of text, but why would I care about the source of a port if I was running an old version of portmaster that doesn't pull from any different sources? Also I don't think we want to broadcast MV sources.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fallback you're referring to targets old theme zips, not old GUI versions. It allows old theme zips to display the new variable. As for description injection, fine to remove if not wanted but the port info list is a small font that can be overlapped by a screenshot. I was going for visibility.
With MV, it's due to the presence of two files. The source comes from the
namekey. I can add a way to merge the two official sources into a single display.