Fix LibGit2 securezero! issue #24731

omus · 2017-11-23T18:51:14Z

Another reminder of the dangers with securezero!. When running on 32-bit machines the GitCredential finalizer was causing copies of Strings to get wiped out. Fixes this issue:

Error During Test at /buildworker/worker/package_linux32/build/usr/share/julia/test/libgit2.jl:2387
  Got an exception of type ErrorException outside of a @test
  Could not locate challenge: "Password for 'https://[email protected]':". Process output found:
  """
  Username for 'https://github.com' [julia]:
  ERROR: ArgumentError: embedded NULs are not allowed in C strings: "Password for 'https://\0\0\0\0\[email protected]':"
  ...
  """

omus · 2017-11-23T18:51:21Z

cc: @staticfloat

omus · 2017-11-23T19:03:44Z

Here's a minimal reproduction of the issue:

julia> git_cred = LibGit2.GitCredential("https", "localhost", nothing, "bob")
Base.LibGit2.GitCredential(Nullable{String}("https"), Nullable{String}("localhost"), Nullable{String}(), Nullable{String}("bob"), Nullable{String}(), true)

julia> u = Base.get(git_cred.username, "")  # Should be using `deepcopy` here
"bob"

julia> git_cred = nothing

julia> gc()

julia> u  # Was intended to be used later
"\0\0\0"

ararslan · 2017-11-23T22:05:38Z

Could you add the reproducing example as a test?

omus · 2017-11-23T22:42:01Z

Could you add the reproducing example as a test?

I should be able to write a test that verifies that garbage collection of the GitCredential in the callback doesn't result in the content of the UserPasswordCredentials also being wiped.

StefanKarpinski · 2017-11-23T22:57:56Z

I’m not sure if this is evidence that securezero! is a good idea or a bad one.

omus · 2017-11-23T23:00:59Z

I’m not sure if this is evidence that securezero! is a good idea or a bad one.

I think the ideal version of this would be to only wipe the string once all references to it have been lost. Wiping a string upon the first reference being dropped is the core issue.

stevengj · 2017-11-24T03:04:03Z

I thought that the consensus in #23232 was to not use a String at all, and just use a Vector{UInt8} for this data?

omus · 2017-11-24T04:45:05Z

I'll be using be switching to Vector{UInt8} instead of String within SecureString for #24738. The current iteration of the code uses String.

omus · 2017-11-24T14:29:35Z

AppVeyor and Travis CI Mac failures look unrelated.

omus · 2017-11-24T14:30:23Z

Any other comments? I'll merge this later today.

stevengj · 2017-11-24T15:13:57Z

test/libgit2.jl

+                    # Triggering a garbage collection will cause the internal GitCredential
+                    # used in `authenticate_userpass` to be securely wiped but this should
+                    # not cause the payload credential to be wiped (#24731).
+                    gc()


Wouldn't it be better to add a manual finalize() call in authenticate_userpass when it is done with the internal GitCredential?

I can add an explicit securezero! call in authenticate_userpass but this gc() will still be required to ensure that the memory has been duplicated.

What do you mean by gc ensuring that memory is duplicated?

What do you mean by gc ensuring that memory is duplicated?

The original issue was that in authenticate_userpass sensitive information was being loaded into git_cred and then shallow copied into a UserPasswordCredentials credential. Since the data was being shallow copied once the git_cred was garbage collected (and then finalized) the data stored within the cred would also be securely wiped.

The gc() in this test case ensures that the git_cred is consistently garbage collected (this was issue was previously only cropping up on 32-bit systems) which will cause the git_cred data to be wiped out. Since the git_cred information is now deep copied to cred the secure wiping of git_cred should not impact the data within cred.

Adding a manual finalize() call would cause the git_cred memory to be wiped sooner (definitely good idea) and would also make sure that cred and git_cred data is independent. My concern with removing the gc() call and only having a finalize() call only is that the conditions to reproduce this issue exist within the source code and not within the tests.

Potentially a future developer could remove deepcopy calls and the finalize calls and the tests would still pass as long as garbage collection hasn't happened to run.

With all of that said having a manual gc() call in a test case is a strange thing to do. If we do consider it bad practise we can go with just using finalize() and hope only me or stevengj modify this code in the future ;)

Just having it in a test case is fine.

@stevengj Good to know. I'll merge as is, without the gc call, so we can get this resolved.

omus · 2017-11-25T02:02:59Z

I squashed the commits and added an explicit securezero! call inside of authenticate_userpass.

omus · 2017-11-25T03:38:50Z

After writing my speech on leaving in the gc() call I decided it was more important just to get this fix merged.

stevengj · 2017-11-25T16:15:15Z

base/libgit2/callbacks.jl

+        # Use `deepcopy` to ensure zeroing the `git_cred` doesn't also zero the `cred`s copy
+        cred.user = deepcopy(Base.get(git_cred.username, ""))
+        cred.pass = deepcopy(Base.get(git_cred.password, ""))
+        securezero!(git_cred)
        revised = true


It seems like we should also call securezero!(cred) at the end of the authenticate_userpass function. The function returns the result of git_cred_userpass_plaintext_new, but this function makes its own copy of the password.

We can't run securezero!(cred) at the end of authenticate_userpass as the credential information is still needed to be used in the approve(::CredentialPayload) and the reject(::CredentialPayload) methods. We should be able to run securezero! on the credentials at the end of the approve/reject methods but I'll need to add in some extra logic to skip zeroing the data when running the test cases.

I was planning to do this in a future PR.

Oh, I see, cred is actually still available via get(p.credential).

omus added the libgit2 The libgit2 library or the LibGit2 stdlib module label Nov 23, 2017

staticfloat approved these changes Nov 23, 2017

View reviewed changes

ararslan added bugfix This change fixes an existing bug needs tests Unit tests are required for this change labels Nov 23, 2017

omus mentioned this pull request Nov 24, 2017

WIP: RFC: Create type SecureString #24738

Closed

omus removed the needs tests Unit tests are required for this change label Nov 24, 2017

stevengj reviewed Nov 24, 2017

View reviewed changes

Fix LibGit2 securezero! issue

b548c8a

omus force-pushed the cv/libgit2-securezero-fix branch from 472f2be to b548c8a Compare November 25, 2017 02:01

omus added 2 commits November 24, 2017 21:33

Secure wipe unused GitCredential

0f2dad8

Avoid dying on the gc hill

7bca2a9

omus merged commit 6fa2047 into master Nov 25, 2017

omus deleted the cv/libgit2-securezero-fix branch November 25, 2017 14:53

stevengj reviewed Nov 25, 2017

View reviewed changes

omus mentioned this pull request Dec 2, 2017

Shred CredentialPayload upon approval/rejection #24884

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix LibGit2 securezero! issue #24731

Fix LibGit2 securezero! issue #24731

omus commented Nov 23, 2017 •

edited

Loading

omus commented Nov 23, 2017

omus commented Nov 23, 2017 •

edited

Loading

ararslan commented Nov 23, 2017

omus commented Nov 23, 2017 •

edited

Loading

StefanKarpinski commented Nov 23, 2017

omus commented Nov 23, 2017 •

edited

Loading

stevengj commented Nov 24, 2017

omus commented Nov 24, 2017

omus commented Nov 24, 2017

omus commented Nov 24, 2017

stevengj Nov 24, 2017 •

edited

Loading

omus Nov 24, 2017

stevengj Nov 24, 2017

omus Nov 25, 2017 •

edited

Loading

omus Nov 25, 2017

omus Nov 25, 2017 •

edited

Loading

stevengj Nov 25, 2017

omus Nov 25, 2017

omus commented Nov 25, 2017

omus commented Nov 25, 2017

stevengj Nov 25, 2017

omus Nov 25, 2017 •

edited

Loading

stevengj Nov 25, 2017

Fix LibGit2 securezero! issue #24731

Fix LibGit2 securezero! issue #24731

Conversation

omus commented Nov 23, 2017 • edited Loading

omus commented Nov 23, 2017

omus commented Nov 23, 2017 • edited Loading

ararslan commented Nov 23, 2017

omus commented Nov 23, 2017 • edited Loading

StefanKarpinski commented Nov 23, 2017

omus commented Nov 23, 2017 • edited Loading

stevengj commented Nov 24, 2017

omus commented Nov 24, 2017

omus commented Nov 24, 2017

omus commented Nov 24, 2017

stevengj Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

omus Nov 24, 2017

Choose a reason for hiding this comment

stevengj Nov 24, 2017

Choose a reason for hiding this comment

omus Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

omus Nov 25, 2017

Choose a reason for hiding this comment

omus Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

stevengj Nov 25, 2017

Choose a reason for hiding this comment

omus Nov 25, 2017

Choose a reason for hiding this comment

omus commented Nov 25, 2017

omus commented Nov 25, 2017

stevengj Nov 25, 2017

Choose a reason for hiding this comment

omus Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

stevengj Nov 25, 2017

Choose a reason for hiding this comment

omus commented Nov 23, 2017 •

edited

Loading

omus commented Nov 23, 2017 •

edited

Loading

omus commented Nov 23, 2017 •

edited

Loading

omus commented Nov 23, 2017 •

edited

Loading

stevengj Nov 24, 2017 •

edited

Loading

omus Nov 25, 2017 •

edited

Loading

omus Nov 25, 2017 •

edited

Loading

omus Nov 25, 2017 •

edited

Loading