-
Notifications
You must be signed in to change notification settings - Fork 18k
[lldb] Load scripts from code signed dSYM bundles #189444
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
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 |
|---|---|---|
|
|
@@ -4371,6 +4371,12 @@ static constexpr OptionEnumValueElement g_load_script_from_sym_file_values[] = { | |
| "warn", | ||
| "Warn about debug scripts inside symbol files but do not load them.", | ||
| }, | ||
| { | ||
| eLoadScriptFromSymFileTrusted, | ||
| "trusted", | ||
|
Member
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 should probably update (or create) some docs describing how this relates to "safe paths". E.g., symbol files from "safe paths" aren't currently |
||
| "Load debug scripts inside trusted symbol files, and warn about " | ||
| "scripts from untrusted symbol files.", | ||
|
Member
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. The There's some logic 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. I think it's important to have a "show me all the things you would load but load none of them" mode. That will help people who are getting used to what all the author of the extensions intends to do. Maybe that's the appropriate use of warn?
Member
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. Yea that's a good point. Though currently when we warn we bail. We should change that to |
||
| }, | ||
| }; | ||
|
|
||
| static constexpr OptionEnumValueElement g_load_cwd_lldbinit_values[] = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| C_SOURCES := main.c | ||
|
|
||
| include Makefile.rules |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| import os | ||
| import shutil | ||
| import subprocess | ||
|
|
||
| import lldb | ||
| from lldbsuite.test.decorators import * | ||
| from lldbsuite.test.lldbtest import * | ||
|
|
||
|
|
||
| def has_lldb_codesign(): | ||
| """Check if the lldb_codesign certificate is available.""" | ||
| try: | ||
| result = subprocess.run( | ||
| [ | ||
| "security", | ||
| "find-certificate", | ||
| "-c", | ||
| "lldb_codesign", | ||
| "/Library/Keychains/System.keychain", | ||
| ], | ||
| capture_output=True, | ||
| ) | ||
| return result.returncode == 0 | ||
| except FileNotFoundError: | ||
| return False | ||
|
|
||
|
|
||
| @skipUnlessDarwin | ||
| class TestdSYMCodesign(TestBase): | ||
| NO_DEBUG_INFO_TESTCASE = True | ||
| SHARED_BUILD_TESTCASE = False | ||
|
|
||
| def build_dsym_with_script(self): | ||
| self.build(debug_info="dsym") | ||
| exe = self.getBuildArtifact("a.out") | ||
| dsym = self.getBuildArtifact("a.out.dSYM") | ||
| python_dir = os.path.join(dsym, "Contents", "Resources", "Python") | ||
| os.makedirs(python_dir, exist_ok=True) | ||
| shutil.copy( | ||
| os.path.join(self.getSourceDir(), "dsym_script.py"), | ||
| os.path.join(python_dir, "a.py"), | ||
| ) | ||
| return exe, dsym | ||
|
|
||
| def test_adhoc_signed_dsym(self): | ||
| """An ad-hoc signed dSYM should not be loaded because the | ||
| signature doesn't chain to a trusted root CA.""" | ||
| exe, dsym = self.build_dsym_with_script() | ||
| subprocess.check_call(["codesign", "-f", "-s", "-", dsym]) | ||
|
|
||
| self.runCmd("settings set target.load-script-from-symbol-file trusted") | ||
| self.createTestTarget(file_path=exe) | ||
|
|
||
| self.expect( | ||
| "script -- print('SENTINEL')", | ||
| substrs=["SENTINEL"], | ||
| ) | ||
| # The script should NOT have been loaded. | ||
| self.assertFalse( | ||
| hasattr(lldb, "_dsym_codesign_test_loaded"), | ||
| "Script should not auto-load from ad-hoc signed dSYM", | ||
| ) | ||
|
|
||
| @unittest.skipUnless(has_lldb_codesign(), "requires lldb_codesign certificate") | ||
| def test_trusted_signed_dsym_auto_loads(self): | ||
| """A dSYM signed with the trusted lldb_codesign certificate should | ||
| auto-load scripts.""" | ||
| exe, dsym = self.build_dsym_with_script() | ||
| subprocess.check_call(["codesign", "-f", "-s", "lldb_codesign", dsym]) | ||
|
|
||
| self.runCmd("settings set target.load-script-from-symbol-file trusted") | ||
| self.createTestTarget(file_path=exe) | ||
|
|
||
| # The script sets a marker attribute on the lldb module. | ||
| self.assertTrue( | ||
| getattr(lldb, "_dsym_codesign_test_loaded", False), | ||
| "Script should auto-load from trusted signed dSYM", | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import lldb | ||
|
|
||
|
|
||
| def __lldb_init_module(debugger, internal_dict): | ||
| lldb._dsym_codesign_test_loaded = True |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| int main(void) { return 0; } |
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.
I'm open to different/more specific naming. I went with "trusted" to cover both the code sign scenario as well as the concept of having default trusted paths, but we could certainly separate the two and have something like
eLoadScriptFromSymFileSignedandeLoadScriptFromSymFileStandardLocationor something.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.
Do we want to distinguish between "system trusted" binaries and 3rd party/ad-hoc signed binaries ?
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.
re separate: Are there systems/circumstances where a user might want both
SignedandStandardLocation?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.
This is starting to feel more like a bit mask. I might want "trusted and no others" or "trusted and warn about the others" for instance.
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.
@medismailben Ad-hoc signed means nothing, so I don't think that's worth having as an option. In theory I can see value in distinguishing between Apple signed vs trusted 3rd party signed, but in practice I think it's simpler to trust what the system is trusting, which is what's currently implemented.
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.
@jimingham Flags would be nice, but AFAIK there's no way to specify flags as a setting.
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.
can we simulate it why an array/list of enum values?