From 41d87d5841767045d398bd441bd86b9ef20df026 Mon Sep 17 00:00:00 2001 From: Curtis Vogt Date: Wed, 30 May 2018 21:06:32 -0500 Subject: [PATCH 01/19] Create type SecureString --- base/exports.jl | 2 + base/strings/secure.jl | 82 +++++++++++ base/strings/strings.jl | 1 + base/util.jl | 10 +- stdlib/LibGit2/src/callbacks.jl | 2 +- stdlib/LibGit2/src/gitcredential.jl | 50 +++---- stdlib/LibGit2/src/types.jl | 48 +++---- stdlib/LibGit2/test/libgit2.jl | 216 +++++++++++++++++++++------- test/strings/secure.jl | 45 ++++++ 9 files changed, 347 insertions(+), 109 deletions(-) create mode 100644 base/strings/secure.jl create mode 100644 test/strings/secure.jl diff --git a/base/exports.jl b/base/exports.jl index 37da399892e43..d3005f29dd7e8 100644 --- a/base/exports.jl +++ b/base/exports.jl @@ -75,6 +75,7 @@ export RoundNearestTiesUp, RoundToZero, RoundUp, + SecureString, Set, Some, StepRange, @@ -586,6 +587,7 @@ export rpad, rsplit, rstrip, + shred!, split, string, strip, diff --git a/base/strings/secure.jl b/base/strings/secure.jl new file mode 100644 index 0000000000000..e8e8eca16719e --- /dev/null +++ b/base/strings/secure.jl @@ -0,0 +1,82 @@ +""" + SecureString(string::AbstractString) + +A string where the contents will be securely wiped when garbage collected. However, it is +considered best practise to wipe the string using `shred!(::SecureString)` as soon as the +secure data is no longer required. Note that when the parameter is of type `Vector{UInt8}` +then the memory of the passed in parameter will also be securely wiped. + +# Examples +```jldoctest +julia> str = "abc"::String +"abc" + +julia> s = SecureString(str) +"abc" + +julia> shred!(s) +"\0\0\0" + +julia> str +"abc" +``` +""" +mutable struct SecureString <: AbstractString + data::Vector{UInt8} + + function SecureString(data::Vector{UInt8}) + s = new(data) + finalizer(final_shred!, s) + return s + end +end + +SecureString(str::String) = SecureString(copy(unsafe_wrap(Vector{UInt8}, str))) +SecureString(str::Union{AbstractString,CodeUnits}) = SecureString(String(str)) + +show(io::IO, s::SecureString) = print(io, "SecureString(\"*****\")") +write(io::IO, s::SecureString) = write(io, s.data) + +function print(io::IO, s::SecureString) + write(io, s.data) + nothing +end + +String(s::SecureString) = String(copy(s.data)) + +iterate(s::SecureString, i::Integer=firstindex(s)) = iterate(String(s), i) +ncodeunits(s::SecureString) = Core.sizeof(s.data) +codeunit(s::SecureString) = UInt8 +codeunit(s::SecureString, i::Integer) = s.data[i] + +isvalid(s::SecureString, i::Int) = isvalid(String(s), i) + +==(a::SecureString, b::SecureString) = a.data == b.data +==(a::String, b::SecureString) = unsafe_wrap(Vector{UInt8}, a) == b.data +==(a::AbstractString, b::SecureString) = String(a) == b +==(a::SecureString, b::AbstractString) = b == a + +function final_shred!(s::SecureString) + if !isshredded(s) + # TODO: Printing SecureString data is temporary while I locate issues in tests + # Note: `@warn` won't work here + println("SecureString \"$s\" not explicitly shredded") + shred!(s) + end +end + +function shred!(s::SecureString) + securezero!(s.data) + return s +end + +isshredded(s::SecureString) = sum(s.data) == 0 + +function shred!(f::Function, x) + try + f(x) + finally + shred!(x) + end + x +end diff --git a/base/strings/strings.jl b/base/strings/strings.jl index 0da4b6312c8ce..6a6eeaa703a03 100644 --- a/base/strings/strings.jl +++ b/base/strings/strings.jl @@ -9,3 +9,4 @@ import .Unicode: textwidth, islowercase, isuppercase, isletter, isdigit, isnumer include("strings/util.jl") include("strings/io.jl") +include("strings/secure.jl") diff --git a/base/util.jl b/base/util.jl index deadac1b1b65e..14ef693f9401f 100644 --- a/base/util.jl +++ b/base/util.jl @@ -447,7 +447,7 @@ securezero!(s::String) = unsafe_securezero!(pointer(s), sizeof(s)) unsafe_securezero!(p::Ptr{Cvoid}, len::Integer=1) = Ptr{Cvoid}(unsafe_securezero!(Ptr{UInt8}(p), len)) if Sys.iswindows() -function getpass(prompt::AbstractString) +function getpass(prompt::AbstractString)::SecureString print(prompt) flush(stdout) p = Vector{UInt8}(undef, 128) # mimic Unix getpass in ignoring more than 128-char passwords @@ -475,11 +475,13 @@ function getpass(prompt::AbstractString) return "" end else -getpass(prompt::AbstractString) = unsafe_string(ccall(:getpass, Cstring, (Cstring,), prompt)) +function getpass(prompt::AbstractString)::SecureString + unsafe_string(ccall(:getpass, Cstring, (Cstring,), prompt)) +end end """ - prompt(message; default="", password=false) -> Union{String, Nothing} + prompt(message; default="", password=false) -> Union{AbstractString, Nothing} Displays the `message` then waits for user input. Input is terminated when a newline (\\n) is encountered or EOF (^D) character is entered on a blank line. If a `default` is provided @@ -583,7 +585,7 @@ if Sys.iswindows() # Done. passbuf_ = passbuf[1:passlen[]-1] result = (String(transcode(UInt8, usernamebuf[1:usernamelen[]-1])), - String(transcode(UInt8, passbuf_))) + SecureString(transcode(UInt8, passbuf_))) securezero!(passbuf_) securezero!(passbuf) diff --git a/stdlib/LibGit2/src/callbacks.jl b/stdlib/LibGit2/src/callbacks.jl index b66973476ac35..da08dfc457804 100644 --- a/stdlib/LibGit2/src/callbacks.jl +++ b/stdlib/LibGit2/src/callbacks.jl @@ -190,7 +190,7 @@ function authenticate_userpass(libgit2credptr::Ptr{Ptr{Cvoid}}, p::CredentialPay # Use `deepcopy` to ensure zeroing the `git_cred` doesn't also zero the `cred`s copy cred.user = deepcopy(something(git_cred.username, "")) cred.pass = deepcopy(something(git_cred.password, "")) - securezero!(git_cred) + shred!(git_cred) revised = true p.use_git_helpers = false diff --git a/stdlib/LibGit2/src/gitcredential.jl b/stdlib/LibGit2/src/gitcredential.jl index 96935f4ec4687..8c185752bd84c 100644 --- a/stdlib/LibGit2/src/gitcredential.jl +++ b/stdlib/LibGit2/src/gitcredential.jl @@ -7,11 +7,11 @@ Git credential information used in communication with git credential helpers. Th named using the [input/output key specification](https://git-scm.com/docs/git-credential#IOFMT). """ mutable struct GitCredential - protocol::Union{String, Nothing} - host::Union{String, Nothing} - path::Union{String, Nothing} - username::Union{String, Nothing} - password::Union{String, Nothing} + protocol::Union{SecureString, Nothing} + host::Union{SecureString, Nothing} + path::Union{SecureString, Nothing} + username::Union{SecureString, Nothing} + password::Union{SecureString, Nothing} use_http_path::Bool function GitCredential( @@ -20,9 +20,7 @@ mutable struct GitCredential path::Union{AbstractString, Nothing}=nothing, username::Union{AbstractString, Nothing}=nothing, password::Union{AbstractString, Nothing}=nothing) - c = new(protocol, host, path, username, password, true) - finalizer(securezero!, c) - return c + new(protocol, host, path, username, password, true) end end @@ -32,17 +30,17 @@ end function GitCredential(cred::UserPasswordCredential, url::AbstractString) git_cred = parse(GitCredential, url) - git_cred.username = deepcopy(cred.user) - git_cred.password = deepcopy(cred.pass) + git_cred.username = SecureString(cred.user) + git_cred.password = SecureString(cred.pass) return git_cred end -function securezero!(cred::GitCredential) - cred.protocol !== nothing && securezero!(cred.protocol) - cred.host !== nothing && securezero!(cred.host) - cred.path !== nothing && securezero!(cred.path) - cred.username !== nothing && securezero!(cred.username) - cred.password !== nothing && securezero!(cred.password) +function shred!(cred::GitCredential) + cred.protocol !== nothing && shred!(cred.protocol) + cred.host !== nothing && shred!(cred.host) + cred.path !== nothing && shred!(cred.path) + cred.username !== nothing && shred!(cred.username) + cred.password !== nothing && shred!(cred.password) return cred end @@ -90,12 +88,11 @@ function Base.parse(::Type{GitCredential}, url::AbstractString) end function Base.copy!(a::GitCredential, b::GitCredential) - # Note: deepcopy calls avoid issues with securezero! - a.protocol = deepcopy(b.protocol) - a.host = deepcopy(b.host) - a.path = deepcopy(b.path) - a.username = deepcopy(b.username) - a.password = deepcopy(b.password) + a.protocol = b.protocol + a.host = b.host + a.path = b.path + a.username = b.username + a.password = b.password return a end @@ -116,9 +113,12 @@ function Base.read!(io::IO, cred::GitCredential) if key == "url" # Any components which are missing from the URL will be set to empty # https://git-scm.com/docs/git-credential#git-credential-codeurlcode + shred!(cred) copy!(cred, parse(GitCredential, value)) else - Base.setproperty!(cred, Symbol(key), String(value)) + field = getproperty(cred, Symbol(key)) + field !== nothing && shred!(field) + setproperty!(cred, Symbol(key), String(value)) end end @@ -283,7 +283,7 @@ function approve(cfg::GitConfig, cred::UserPasswordCredential, url::AbstractStri approve(helper, git_cred) end - securezero!(git_cred) + shred!(git_cred) nothing end @@ -295,6 +295,6 @@ function reject(cfg::GitConfig, cred::UserPasswordCredential, url::AbstractStrin reject(helper, git_cred) end - securezero!(git_cred) + shred!(git_cred) nothing end diff --git a/stdlib/LibGit2/src/types.jl b/stdlib/LibGit2/src/types.jl index a58b660d5a099..8ccfd5941a6c6 100644 --- a/stdlib/LibGit2/src/types.jl +++ b/stdlib/LibGit2/src/types.jl @@ -1180,7 +1180,7 @@ function objtype(obj_type::Consts.OBJECT) end end -import Base.securezero! +import Base.shred! abstract type AbstractCredential end @@ -1193,12 +1193,10 @@ isfilled(::AbstractCredential) "Credential that support only `user` and `password` parameters" mutable struct UserPasswordCredential <: AbstractCredential - user::String - pass::String + user::SecureString + pass::SecureString function UserPasswordCredential(user::AbstractString="", pass::AbstractString="") - c = new(user, pass) - finalizer(securezero!, c) - return c + new(user, pass) end # Deprecated constructors @@ -1212,9 +1210,9 @@ mutable struct UserPasswordCredential <: AbstractCredential UserPasswordCredential(prompt_if_incorrect::Bool) = UserPasswordCredential("","",prompt_if_incorrect) end -function securezero!(cred::UserPasswordCredential) - securezero!(cred.user) - securezero!(cred.pass) +function shred!(cred::UserPasswordCredential) + shred!(cred.user) + shred!(cred.pass) return cred end @@ -1228,15 +1226,13 @@ end "SSH credential type" mutable struct SSHCredential <: AbstractCredential - user::String - pass::String - prvkey::String - pubkey::String + user::SecureString + pass::SecureString + prvkey::SecureString + pubkey::SecureString function SSHCredential(user::AbstractString="", pass::AbstractString="", - prvkey::AbstractString="", pubkey::AbstractString="") - c = new(user, pass, prvkey, pubkey) - finalizer(securezero!, c) - return c + prvkey::AbstractString="", pubkey::AbstractString="") + new(user, pass, prvkey, pubkey) end # Deprecated constructors @@ -1251,11 +1247,11 @@ mutable struct SSHCredential <: AbstractCredential SSHCredential(prompt_if_incorrect::Bool) = SSHCredential("","","","",prompt_if_incorrect) end -function securezero!(cred::SSHCredential) - securezero!(cred.user) - securezero!(cred.pass) - securezero!(cred.prvkey) - securezero!(cred.pubkey) +function shred!(cred::SSHCredential) + shred!(cred.user) + shred!(cred.pass) + shred!(cred.prvkey) + shred!(cred.pubkey) return cred end @@ -1278,8 +1274,8 @@ Base.haskey(cache::CachedCredentials, cred_id) = Base.haskey(cache.cred, cred_id Base.getindex(cache::CachedCredentials, cred_id) = Base.getindex(cache.cred, cred_id) Base.get!(cache::CachedCredentials, cred_id, default) = Base.get!(cache.cred, cred_id, default) -function securezero!(p::CachedCredentials) - foreach(securezero!, values(p.cred)) +function shred!(p::CachedCredentials) + foreach(shred!, values(p.cred)) return p end @@ -1393,7 +1389,7 @@ function approve(p::CredentialPayload; shred::Bool=true) approve(p.config, cred, p.url) end - shred && securezero!(cred) + shred && shred!(cred) nothing end @@ -1418,7 +1414,7 @@ function reject(p::CredentialPayload; shred::Bool=true) reject(p.config, cred, p.url) end - shred && securezero!(cred) + shred && shred!(cred) nothing end diff --git a/stdlib/LibGit2/test/libgit2.jl b/stdlib/LibGit2/test/libgit2.jl index b937bd48b762d..131cb33053b44 100644 --- a/stdlib/LibGit2/test/libgit2.jl +++ b/stdlib/LibGit2/test/libgit2.jl @@ -451,9 +451,13 @@ end username=alice password=***** """ + expected_cred = LibGit2.GitCredential("https", "example.com", nothing, "alice", "*****") + cred = read!(IOBuffer(str), LibGit2.GitCredential()) - @test cred == LibGit2.GitCredential("https", "example.com", nothing, "alice", "*****") + @test cred == expected_cred @test sprint(write, cred) == str + shred!(cred) + shred!(expected_cred) end @testset "use http path" begin @@ -464,11 +468,13 @@ end username=alice password=***** """ + @test cred.use_http_path cred.use_http_path = false @test cred.path == "dir/file" @test sprint(write, cred) == expected + shred!(cred) end @testset "URL input/output" begin @@ -478,40 +484,50 @@ end url=https://a@b/c username=foo """ - expected = """ + expected_str = """ protocol=https host=b path=c username=foo """ + expected_cred = LibGit2.GitCredential("https", "b", "c", "foo", nothing) + cred = read!(IOBuffer(str), LibGit2.GitCredential()) - @test cred == LibGit2.GitCredential("https", "b", "c", "foo", nothing) - @test sprint(write, cred) == expected + @test cred == expected_cred + @test sprint(write, cred) == expected_str + shred!(cred) + shred!(expected_cred) end @testset "ismatch" begin # Equal cred = LibGit2.GitCredential("https", "github.com") @test LibGit2.ismatch("https://github.com", cred) + shred!(cred) # Credential hostname is different cred = LibGit2.GitCredential("https", "github.com") @test !LibGit2.ismatch("https://myhost", cred) + shred!(cred) # Credential is less specific than URL cred = LibGit2.GitCredential("https") @test !LibGit2.ismatch("https://github.com", cred) + shred!(cred) # Credential is more specific than URL cred = LibGit2.GitCredential("https", "github.com", "path", "user", "pass") @test LibGit2.ismatch("https://github.com", cred) + shred!(cred) # Credential needs to have an "" username to match cred = LibGit2.GitCredential("https", "github.com", nothing, "") @test LibGit2.ismatch("https://@github.com", cred) + shred!(cred) cred = LibGit2.GitCredential("https", "github.com", nothing, nothing) @test !LibGit2.ismatch("https://@github.com", cred) + shred!(cred) end end @@ -1664,6 +1680,7 @@ mktempdir() do dir @test creds.pass == creds_pass creds2 = LibGit2.UserPasswordCredential(creds_user, creds_pass) @test creds == creds2 + sshcreds = LibGit2.SSHCredential(creds_user, creds_pass) @test sshcreds.user == creds_user @test sshcreds.pass == creds_pass @@ -1671,6 +1688,11 @@ mktempdir() do dir @test isempty(sshcreds.pubkey) sshcreds2 = LibGit2.SSHCredential(creds_user, creds_pass) @test sshcreds == sshcreds2 + + shred!(creds) + shred!(creds2) + shred!(sshcreds) + shred!(sshcreds2) end @testset "CachedCredentials" begin @@ -1698,6 +1720,9 @@ mktempdir() do dir @test !haskey(cache, cred_id) @test cred.user == "julia" @test cred.pass == "password" + + shred!(cache) + shred!(cred) end @testset "Git credential username" begin @@ -1709,33 +1734,34 @@ mktempdir() do dir # No credential settings should be set for these tests @test isempty(collect(LibGit2.GitConfigIter(cfg, r"credential.*"))) + github_cred = LibGit2.GitCredential("https", "github.com") + mygit_cred = LibGit2.GitCredential("https", "mygithost") + # No credential settings in configuration. - cred = LibGit2.GitCredential("https", "github.com") - username = LibGit2.default_username(cfg, cred) + username = LibGit2.default_username(cfg, github_cred) @test username === nothing # Add a credential setting for a specific for a URL LibGit2.set!(cfg, "credential.https://jackfan.us.kg.username", "foo") - cred = LibGit2.GitCredential("https", "github.com") - username = LibGit2.default_username(cfg, cred) + username = LibGit2.default_username(cfg, github_cred) @test username == "foo" - cred = LibGit2.GitCredential("https", "mygithost") - username = LibGit2.default_username(cfg, cred) + username = LibGit2.default_username(cfg, mygit_cred) @test username === nothing # Add a global credential setting after the URL specific setting. The first # setting to match will be the one that is used. LibGit2.set!(cfg, "credential.username", "bar") - cred = LibGit2.GitCredential("https", "github.com") - username = LibGit2.default_username(cfg, cred) + username = LibGit2.default_username(cfg, github_cred) @test username == "foo" - cred = LibGit2.GitCredential("https", "mygithost") - username = LibGit2.default_username(cfg, cred) + username = LibGit2.default_username(cfg, mygit_cred) @test username == "bar" + + shred!(github_cred) + shred!(mygit_cred) end end @@ -1751,13 +1777,17 @@ mktempdir() do dir LibGit2.set!(cfg, "credential.https://jackfan.us.kg.username", "") LibGit2.set!(cfg, "credential.username", "name") - cred = LibGit2.GitCredential("https", "github.com") - username = LibGit2.default_username(cfg, cred) + github_cred = LibGit2.GitCredential("https", "github.com") + mygit_cred = LibGit2.GitCredential("https", "mygithost", "path") + + username = LibGit2.default_username(cfg, github_cred) @test username == "" - cred = LibGit2.GitCredential("https", "mygithost", "path") - username = LibGit2.default_username(cfg, cred) + username = LibGit2.default_username(cfg, mygit_cred) @test username == "name" + + shred!(github_cred) + shred!(mygit_cred) end end end @@ -1771,28 +1801,28 @@ mktempdir() do dir # No credential settings should be set for these tests @test isempty(collect(LibGit2.GitConfigIter(cfg, r"credential.*"))) + github_cred = LibGit2.GitCredential("https", "github.com") + mygit_cred = LibGit2.GitCredential("https", "mygithost") + # No credential settings in configuration. - cred = LibGit2.GitCredential("https", "github.com") - @test !LibGit2.use_http_path(cfg, cred) + @test !LibGit2.use_http_path(cfg, github_cred) + @test !LibGit2.use_http_path(cfg, mygit_cred) # Add a credential setting for a specific for a URL LibGit2.set!(cfg, "credential.https://jackfan.us.kg.useHttpPath", "true") - cred = LibGit2.GitCredential("https", "github.com") - @test LibGit2.use_http_path(cfg, cred) - - cred = LibGit2.GitCredential("https", "mygithost") - @test !LibGit2.use_http_path(cfg, cred) + @test LibGit2.use_http_path(cfg, github_cred) + @test !LibGit2.use_http_path(cfg, mygit_cred) # Invert the current settings. LibGit2.set!(cfg, "credential.useHttpPath", "true") LibGit2.set!(cfg, "credential.https://jackfan.us.kg.useHttpPath", "false") - cred = LibGit2.GitCredential("https", "github.com") - @test !LibGit2.use_http_path(cfg, cred) + @test !LibGit2.use_http_path(cfg, github_cred) + @test LibGit2.use_http_path(cfg, mygit_cred) - cred = LibGit2.GitCredential("https", "mygithost") - @test LibGit2.use_http_path(cfg, cred) + shred!(github_cred) + shred!(mygit_cred) end end end @@ -1830,10 +1860,16 @@ mktempdir() do dir GitCredentialHelper(`echo second`), ] - @test LibGit2.credential_helpers(cfg, GitCredential("https", "github.com")) == expected + github_cred = GitCredential("https", "github.com") + mygit_cred = GitCredential("https", "mygithost") + + @test LibGit2.credential_helpers(cfg, github_cred) == expected println(stderr, "The following 'Resetting the helper list...' warning is expected:") - @test_broken LibGit2.credential_helpers(cfg, GitCredential("https", "mygithost")) == expected[2] + @test_broken LibGit2.credential_helpers(cfg, mygit_cred) == expected[2] + + shred!(github_cred) + shred!(mygit_cred) end end @@ -1856,14 +1892,23 @@ mktempdir() do dir @test !isfile(credential_path) - @test LibGit2.fill!(helper, deepcopy(query)) == query + shred!(LibGit2.fill!(helper, deepcopy(query))) do result + @test result == query + end LibGit2.approve(helper, filled) @test isfile(credential_path) - @test LibGit2.fill!(helper, deepcopy(query)) == filled + shred!(LibGit2.fill!(helper, deepcopy(query))) do result + @test result == filled + end LibGit2.reject(helper, filled) - @test LibGit2.fill!(helper, deepcopy(query)) == query + shred!(LibGit2.fill!(helper, deepcopy(query))) do result + @test result == query + end + + shred!(query) + shred!(filled) end end end @@ -1895,27 +1940,62 @@ mktempdir() do dir c end + filled_without_path_a = without_path(filled_a) + filled_without_path_b = without_path(filled_b) + @test !isfile(credential_path) - @test LibGit2.fill!(helper, deepcopy(query)) == query - @test LibGit2.fill!(helper, deepcopy(query_a)) == query_a - @test LibGit2.fill!(helper, deepcopy(query_b)) == query_b + shred!(LibGit2.fill!(helper, deepcopy(query))) do result + @test result == query + end + shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result + @test result == query_a + end + shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result + @test result == query_b + end LibGit2.approve(helper, filled_a) @test isfile(credential_path) - @test LibGit2.fill!(helper, deepcopy(query)) == without_path(filled_a) - @test LibGit2.fill!(helper, deepcopy(query_a)) == filled_a - @test LibGit2.fill!(helper, deepcopy(query_b)) == query_b + shred!(LibGit2.fill!(helper, deepcopy(query))) do result + @test result == filled_without_path_a + end + shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result + @test result == filled_a + end + shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result + @test result == query_b + end LibGit2.approve(helper, filled_b) - @test LibGit2.fill!(helper, deepcopy(query)) == without_path(filled_b) - @test LibGit2.fill!(helper, deepcopy(query_a)) == filled_a - @test LibGit2.fill!(helper, deepcopy(query_b)) == filled_b + shred!(LibGit2.fill!(helper, deepcopy(query))) do result + @test result == filled_without_path_b + end + shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result + @test result == filled_a + end + shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result + @test result == filled_b + end LibGit2.reject(helper, filled_b) - @test LibGit2.fill!(helper, deepcopy(query)) == without_path(filled_a) - @test LibGit2.fill!(helper, deepcopy(query_a)) == filled_a - @test LibGit2.fill!(helper, deepcopy(query_b)) == query_b + shred!(LibGit2.fill!(helper, deepcopy(query))) do result + @test result == filled_without_path_a + end + shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result + @test result == filled_a + end + shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result + @test result == query_b + end + + shred!(query) + shred!(query_a) + shred!(query_b) + shred!(filled_a) + shred!(filled_b) + shred!(filled_without_path_a) + shred!(filled_without_path_b) end end end @@ -2186,6 +2266,9 @@ mktempdir() do dir @test err == git_ok @test auth_attempts == 2 end + + shred!(valid_cred) + shred!(valid_p_cred) end @testset "HTTPS credential prompt" begin @@ -2249,6 +2332,8 @@ mktempdir() do dir err, auth_attempts, p = challenge_prompt(https_ex, challenges) @test err == prompt_limit @test auth_attempts == 3 + + shred!(valid_cred) end @testset "SSH agent username" begin @@ -2278,6 +2363,8 @@ mktempdir() do dir err, auth_attempts, p = challenge_prompt(ex, []) @test err == exhausted_error @test auth_attempts == 2 + + shred!(valid_cred) end @testset "SSH default" begin @@ -2338,6 +2425,9 @@ mktempdir() do dir @test err == git_ok @test auth_attempts == 1 end + + shred!(valid_cred) + shred!(valid_p_cred) end end @@ -2386,6 +2476,8 @@ mktempdir() do dir @test auth_attempts == 2 @test p.credential.pubkey == abspath(valid_key * ".pub") end + + shred!(valid_cred) end @testset "SSH explicit credentials" begin @@ -2425,6 +2517,9 @@ mktempdir() do dir @test auth_attempts == 3 @test p.explicit == invalid_cred @test p.credential != invalid_cred + + shred!(valid_cred) + shred!(invalid_cred) end @testset "HTTPS explicit credentials" begin @@ -2457,6 +2552,9 @@ mktempdir() do dir @test auth_attempts == 2 @test p.explicit == invalid_cred @test p.credential != invalid_cred + + shred!(valid_cred) + shred!(invalid_cred) end @testset "Cached credentials" begin @@ -2541,6 +2639,9 @@ mktempdir() do dir @test typeof(cache) == LibGit2.CachedCredentials @test cache.cred == Dict() @test p.credential != invalid_cred + + shred!(valid_cred) + shred!(invalid_cred) end @testset "HTTPS git helper username" begin @@ -2577,6 +2678,8 @@ mktempdir() do dir # Verify credential wasn't accidentally zeroed (#24731) @test p.credential == valid_cred + + shred!(valid_cred) end @testset "Incompatible explicit credentials" begin @@ -2596,6 +2699,7 @@ mktempdir() do dir @test p.explicit == valid_cred @test p.credential != valid_cred + shred!(valid_cred) # User provides a SSH credential where a user/password credential is required. valid_cred = LibGit2.SSHCredential("foo", "", "", "") @@ -2612,6 +2716,8 @@ mktempdir() do dir @test auth_attempts == 1 @test p.explicit == valid_cred @test p.credential != valid_cred + + shred!(valid_cred) end # A hypothetical scenario where the the allowed authentication can either be @@ -2621,18 +2727,20 @@ mktempdir() do dir Cuint(LibGit2.Consts.CREDTYPE_USERPASS_PLAINTEXT) # User provides a user/password credential where a SSH credential is required. + valid_cred = LibGit2.UserPasswordCredential("foo", "bar") ex = quote include($LIBGIT2_HELPER_PATH) - valid_cred = LibGit2.UserPasswordCredential("foo", "bar") - payload = CredentialPayload(valid_cred, allow_ssh_agent=false, + payload = CredentialPayload($valid_cred, allow_ssh_agent=false, allow_git_helpers=false) - credential_loop(valid_cred, "foo://github.com/repo", "", + credential_loop($valid_cred, "foo://github.com/repo", "", $allowed_types, payload) end err, auth_attempts, p = challenge_prompt(ex, []) @test err == git_ok @test auth_attempts == 1 + + shred!(valid_cred) end @testset "CredentialPayload reset" begin @@ -2643,16 +2751,16 @@ mktempdir() do dir valid_username = "julia" valid_password = randstring(16) + valid_cred = LibGit2.UserPasswordCredential(valid_username, valid_password) # Users should be able to re-use the same payload if the state is reset ex = quote include($LIBGIT2_HELPER_PATH) - valid_cred = LibGit2.UserPasswordCredential($valid_username, $valid_password) user = nothing payload = CredentialPayload(allow_git_helpers=false) - first_result = credential_loop(valid_cred, $(urls[1]), user, payload) + first_result = credential_loop($valid_cred, $(urls[1]), user, payload) LibGit2.reset!(payload) - second_result = credential_loop(valid_cred, $(urls[2]), user, payload) + second_result = credential_loop($valid_cred, $(urls[2]), user, payload) (first_result, second_result) end @@ -2671,6 +2779,8 @@ mktempdir() do dir err, auth_attempts, p = second_result @test err == git_ok @test auth_attempts == 1 + + shred!(valid_cred) end end @@ -2791,7 +2901,7 @@ end let cache = LibGit2.CachedCredentials() get!(cache, "foo", LibGit2.SSHCredential("", "bar")) - Base.securezero!(cache) + shred!(cache) @test cache["foo"].pass == "\0\0\0" end diff --git a/test/strings/secure.jl b/test/strings/secure.jl new file mode 100644 index 0000000000000..fcea253605dbb --- /dev/null +++ b/test/strings/secure.jl @@ -0,0 +1,45 @@ +## Secure strings ## + +@testset "SecureString" begin + @testset "wipe original" begin + str = String("foobar") + secure = SecureString(str) + @test str == secure + + # Securely wiping the SecureString will also wipe out the original source + shred!(secure) === secure + @test secure != "foobar" + @test str != "foobar" + end + + @testset "finalizer" begin + # Note: Using `@test_warn` ends up messing up the test + finalizer_test() = begin + str = "foobar" + expected = deepcopy(str) + secure_a = SecureString(str) + secure_b = secure_a + + secure_a = nothing + gc() + + @test str == expected # Still retaining a reference to the original SecureString + + secure_b = nothing + gc() + + @test str != expected + end + + @test_warn "SecureString \"foobar\" not explicitly shredded" finalizer_test() + end + + @testset "deepcopy" begin + secure_a = SecureString("foo") + secure_b = deepcopy(secure_a) + shred!(secure_a) + + @test secure_a != "foo" + @test secure_b == "foo" + end +end From 8abc616fb0c27b56813da442e55ee9c752025167 Mon Sep 17 00:00:00 2001 From: Curtis Vogt Date: Thu, 31 May 2018 00:38:36 -0500 Subject: [PATCH 02/19] Switch `GitCredential` equality to use `==` Originally used `isequal` to deal with `Nullable` --- base/strings/secure.jl | 102 +++++++++++++++++++++------- base/util.jl | 42 +++++------- stdlib/LibGit2/src/callbacks.jl | 9 ++- stdlib/LibGit2/src/gitcredential.jl | 61 ++++++++--------- stdlib/LibGit2/src/types.jl | 19 +++--- stdlib/LibGit2/src/utils.jl | 25 ++++--- stdlib/LibGit2/test/libgit2.jl | 47 +++++++------ stdlib/LibGit2/test/online.jl | 1 + 8 files changed, 177 insertions(+), 129 deletions(-) diff --git a/base/strings/secure.jl b/base/strings/secure.jl index e8e8eca16719e..b4715cd18bc5d 100644 --- a/base/strings/secure.jl +++ b/base/strings/secure.jl @@ -21,56 +21,108 @@ julia> str "abc" ``` """ -mutable struct SecureString <: AbstractString +mutable struct SecureString <: IO data::Vector{UInt8} + size::Int + ptr::Int - function SecureString(data::Vector{UInt8}) - s = new(data) + function SecureString(; sizehint=128) + s = new(Vector{UInt8}(undef, sizehint), 0, 1) finalizer(final_shred!, s) return s end end -SecureString(str::String) = SecureString(copy(unsafe_wrap(Vector{UInt8}, str))) -SecureString(str::Union{AbstractString,CodeUnits}) = SecureString(String(str)) - -show(io::IO, s::SecureString) = print(io, "SecureString(\"*****\")") -write(io::IO, s::SecureString) = write(io, s.data) +function SecureString!(p::Ptr{UInt8}) + # Copy into our own Vector and zero the source + len = ccall(:strlen, Cint, (Ptr{UInt8},), p) + s = SecureString(sizehint=len) + for i in 1:len + write(s, unsafe_load(p, i)) + end + seek(s, 0) + s +end -function print(io::IO, s::SecureString) - write(io, s.data) - nothing +convert(::Type{SecureString}, s::AbstractString) = SecureString(String(s)) +SecureString(str::AbstractString) = SecureString(String(str)) +function SecureString(str::String) + buf = unsafe_wrap(Vector{UInt8}, str) + s = SecureString(sizehint=length(buf)) + for c in buf + write(s, c) + end + seek(s, 0) + s end -String(s::SecureString) = String(copy(s.data)) +show(io::IO, s::SecureString) = print(io, "SecureString(\"*******\")") -iterate(s::SecureString, i::Integer=firstindex(s)) = iterate(String(s), i) -ncodeunits(s::SecureString) = Core.sizeof(s.data) -codeunit(s::SecureString) = UInt8 -codeunit(s::SecureString, i::Integer) = s.data[i] +hash(s::SecureString, h::UInt) = hash(SecureString, hash((s.data, s.size, s.ptr), h)) +==(s1::SecureString, s2::SecureString) = (view(s1.data, 1:s1.size), s1.ptr) == (view(s2.data, 1:s2.size), s2.ptr) -isvalid(s::SecureString, i::Int) = isvalid(String(s), i) +function write(io::SecureString, b::UInt8) + if io.ptr > length(io.data) + # We need to resize! the array: do this manually to ensure no copies are left behind + newdata = Vector{UInt8}(undef, (io.size+16)*2) + copyto!(newdata, io.data) + securezero!(io.data) + io.data = newdata + end + io.size == io.ptr-1 && (io.size += 1) + io.data[io.ptr] = b + io.ptr += 1 + return 1 +end -==(a::SecureString, b::SecureString) = a.data == b.data -==(a::String, b::SecureString) = unsafe_wrap(Vector{UInt8}, a) == b.data -==(a::AbstractString, b::SecureString) = String(a) == b -==(a::SecureString, b::AbstractString) = b == a +function write(io::IO, s::SecureString) + nb = 0 + for i in 1:s.size + nb += write(io, s.data[i]) + end + return nb +end + +function unsafe_convert(::Type{Ptr{UInt8}}, s::SecureString) + # Add a hidden nul byte just past the end of the valid region + p = s.ptr + s.ptr = s.size + 1 + write(s, '\0') + s.ptr = p + s.size -= 1 + return unsafe_convert(Ptr{UInt8}, s.data) +end + +seek(io::SecureString, n::Integer) = (io.ptr = max(min(n+1, io.size+1), 1)) +bytesavailable(io::SecureString) = io.size - io.ptr + 1 +position(io::SecureString) = io.ptr-1 +eof(io::SecureString) = io.ptr > io.size +isempty(io::SecureString) = io.size == 0 +function peek(io::SecureString) + eof(io) && throw(EOFError()) + return io.data[io.ptr] +end +function read(io::SecureString, ::Type{UInt8}) + eof(io) && throw(EOFError()) + byte = io.data[io.ptr] + io.ptr += 1 + return byte +end function final_shred!(s::SecureString) if !isshredded(s) - # TODO: Printing SecureString data is temporary while I locate issues in tests - # Note: `@warn` won't work here - println("SecureString \"$s\" not explicitly shredded") shred!(s) end end function shred!(s::SecureString) securezero!(s.data) + s.ptr = 1 + s.size = 0 return s end -isshredded(s::SecureString) = sum(s.data) == 0 +isshredded(s::SecureString) = all(iszero, s.data) function shred!(f::Function, x) try diff --git a/base/util.jl b/base/util.jl index 14ef693f9401f..f1ec3a2b2e9b5 100644 --- a/base/util.jl +++ b/base/util.jl @@ -441,42 +441,34 @@ will always be called. """ function securezero! end @noinline securezero!(a::AbstractArray{<:Number}) = fill!(a, 0) -securezero!(s::String) = unsafe_securezero!(pointer(s), sizeof(s)) +securezero!(s::Union{String,Cstring}) = unsafe_securezero!(pointer(s), sizeof(s)) @noinline unsafe_securezero!(p::Ptr{T}, len::Integer=1) where {T} = ccall(:memset, Ptr{T}, (Ptr{T}, Cint, Csize_t), p, 0, len*sizeof(T)) unsafe_securezero!(p::Ptr{Cvoid}, len::Integer=1) = Ptr{Cvoid}(unsafe_securezero!(Ptr{UInt8}(p), len)) if Sys.iswindows() -function getpass(prompt::AbstractString)::SecureString +function getpass(prompt::AbstractString) print(prompt) flush(stdout) - p = Vector{UInt8}(undef, 128) # mimic Unix getpass in ignoring more than 128-char passwords - # (also avoids any potential memory copies arising from push!) - try - plen = 0 - while true - c = ccall(:_getch, UInt8, ()) - if c == 0xff || c == UInt8('\n') || c == UInt8('\r') - break # EOF or return - elseif c == 0x00 || c == 0xe0 - ccall(:_getch, UInt8, ()) # ignore function/arrow keys - elseif c == UInt8('\b') && plen > 0 - plen -= 1 # delete last character on backspace - elseif !iscntrl(Char(c)) && plen < 128 - p[plen += 1] = c - end + s = SecureString() + plen = 0 + while true + c = ccall(:_getch, UInt8, ()) + if c == 0xff || c == UInt8('\n') || c == UInt8('\r') + break # EOF or return + elseif c == 0x00 || c == 0xe0 + ccall(:_getch, UInt8, ()) # ignore function/arrow keys + elseif c == UInt8('\b') && plen > 0 + plen -= 1 # delete last character on backspace + elseif !iscntrl(Char(c)) && plen < 128 + write(s, c) end - return unsafe_string(pointer(p), plen) # use unsafe_string rather than String(p[1:plen]) - # to be absolutely certain we never make an extra copy - finally - securezero!(p) end - - return "" + return s end else -function getpass(prompt::AbstractString)::SecureString - unsafe_string(ccall(:getpass, Cstring, (Cstring,), prompt)) +function getpass(prompt::AbstractString) + SecureString(ccall(:getpass, Cstring, (Cstring,), prompt)) end end diff --git a/stdlib/LibGit2/src/callbacks.jl b/stdlib/LibGit2/src/callbacks.jl index da08dfc457804..88e8ccf4df5a2 100644 --- a/stdlib/LibGit2/src/callbacks.jl +++ b/stdlib/LibGit2/src/callbacks.jl @@ -169,7 +169,7 @@ function authenticate_ssh(libgit2credptr::Ptr{Ptr{Cvoid}}, p::CredentialPayload, end return ccall((:git_cred_ssh_key_new, :libgit2), Cint, - (Ptr{Ptr{Cvoid}}, Cstring, Cstring, Cstring, Cstring), + (Ptr{Ptr{Cvoid}}, Cstring, Cstring, Cstring, Ptr{UInt8}), libgit2credptr, cred.user, cred.pubkey, cred.prvkey, cred.pass) end @@ -187,9 +187,8 @@ function authenticate_userpass(libgit2credptr::Ptr{Ptr{Cvoid}}, p::CredentialPay if p.use_git_helpers && (!revised || !isfilled(cred)) git_cred = GitCredential(p.config, p.url) - # Use `deepcopy` to ensure zeroing the `git_cred` doesn't also zero the `cred`s copy - cred.user = deepcopy(something(git_cred.username, "")) - cred.pass = deepcopy(something(git_cred.password, "")) + cred.user = something(git_cred.username, "") + cred.pass = something(git_cred.password, "") shred!(git_cred) revised = true @@ -228,7 +227,7 @@ function authenticate_userpass(libgit2credptr::Ptr{Ptr{Cvoid}}, p::CredentialPay end return ccall((:git_cred_userpass_plaintext_new, :libgit2), Cint, - (Ptr{Ptr{Cvoid}}, Cstring, Cstring), + (Ptr{Ptr{Cvoid}}, Cstring, Ptr{UInt8}), libgit2credptr, cred.user, cred.pass) end diff --git a/stdlib/LibGit2/src/gitcredential.jl b/stdlib/LibGit2/src/gitcredential.jl index 8c185752bd84c..ccb21aa757d44 100644 --- a/stdlib/LibGit2/src/gitcredential.jl +++ b/stdlib/LibGit2/src/gitcredential.jl @@ -7,10 +7,10 @@ Git credential information used in communication with git credential helpers. Th named using the [input/output key specification](https://git-scm.com/docs/git-credential#IOFMT). """ mutable struct GitCredential - protocol::Union{SecureString, Nothing} - host::Union{SecureString, Nothing} - path::Union{SecureString, Nothing} - username::Union{SecureString, Nothing} + protocol::Union{AbstractString, Nothing} + host::Union{AbstractString, Nothing} + path::Union{AbstractString, Nothing} + username::Union{AbstractString, Nothing} password::Union{SecureString, Nothing} use_http_path::Bool @@ -28,30 +28,21 @@ function GitCredential(cfg::GitConfig, url::AbstractString) fill!(cfg, parse(GitCredential, url)) end -function GitCredential(cred::UserPasswordCredential, url::AbstractString) - git_cred = parse(GitCredential, url) - git_cred.username = SecureString(cred.user) - git_cred.password = SecureString(cred.pass) - return git_cred -end +GitCredential(cred::UserPasswordCredential, url::AbstractString) = parse(GitCredential, url) + +Base.:(==)(c1::GitCredential, c2::GitCredential) = (c1.protocol, c1.host, c1.path, c1.username, c1.password, c1.use_http_path) == + (c2.protocol, c2.host, c2.path, c2.username, c2.password, c2.use_http_path) +Base.hash(cred::GitCredential, h::UInt) = hash(GitCredential, hash((cred.protocol, cred.host, cred.path, cred.username, cred.password, cred.use_http_path), h)) function shred!(cred::GitCredential) - cred.protocol !== nothing && shred!(cred.protocol) - cred.host !== nothing && shred!(cred.host) - cred.path !== nothing && shred!(cred.path) - cred.username !== nothing && shred!(cred.username) + cred.protocol !== nothing && (cred.protocol = nothing) + cred.host !== nothing && (cred.host = nothing) + cred.path !== nothing && (cred.path = nothing) + cred.username !== nothing && (cred.username = nothing) cred.password !== nothing && shred!(cred.password) return cred end -function Base.:(==)(a::GitCredential, b::GitCredential) - isequal(a.protocol, b.protocol) && - isequal(a.host, b.host) && - isequal(a.path, b.path) && - isequal(a.username, b.username) && - isequal(a.password, b.password) && - a.use_http_path == b.use_http_path -end """ ismatch(url, git_cred) -> Bool @@ -97,19 +88,27 @@ function Base.copy!(a::GitCredential, b::GitCredential) end function Base.write(io::IO, cred::GitCredential) - cred.protocol !== nothing && println(io, "protocol=", cred.protocol) - cred.host !== nothing && println(io, "host=", cred.host) - cred.path !== nothing && cred.use_http_path && println(io, "path=", cred.path) - cred.username !== nothing && println(io, "username=", cred.username) - cred.password !== nothing && println(io, "password=", cred.password) + cred.protocol !== nothing && write(io, "protocol=", cred.protocol, '\n') + cred.host !== nothing && write(io, "host=", cred.host, '\n') + cred.path !== nothing && cred.use_http_path && write(io, "path=", cred.path, '\n') + cred.username !== nothing && write(io, "username=", cred.username, '\n') + cred.password !== nothing && write(io, "password=", cred.password, '\n') nothing end function Base.read!(io::IO, cred::GitCredential) # https://git-scm.com/docs/git-credential#IOFMT while !eof(io) - key, value = split(readline(io), '=') - + key = readuntil(io, '=') + if key == "password" + value = SecureString() + while !eof(io) && (c = read(io, UInt8)) != UInt8('\n') + write(value, c) + end + seek(value, 0) + else + value = readuntil(io, '\n') + end if key == "url" # Any components which are missing from the URL will be set to empty # https://git-scm.com/docs/git-credential#git-credential-codeurlcode @@ -117,8 +116,8 @@ function Base.read!(io::IO, cred::GitCredential) copy!(cred, parse(GitCredential, value)) else field = getproperty(cred, Symbol(key)) - field !== nothing && shred!(field) - setproperty!(cred, Symbol(key), String(value)) + field !== nothing && Symbol(key) == :password && shred!(field) + setproperty!(cred, Symbol(key), value) end end diff --git a/stdlib/LibGit2/src/types.jl b/stdlib/LibGit2/src/types.jl index 8ccfd5941a6c6..4d0b182a2fc32 100644 --- a/stdlib/LibGit2/src/types.jl +++ b/stdlib/LibGit2/src/types.jl @@ -1193,9 +1193,9 @@ isfilled(::AbstractCredential) "Credential that support only `user` and `password` parameters" mutable struct UserPasswordCredential <: AbstractCredential - user::SecureString + user::AbstractString pass::SecureString - function UserPasswordCredential(user::AbstractString="", pass::AbstractString="") + function UserPasswordCredential(user::AbstractString="", pass::Union{AbstractString, SecureString}="") new(user, pass) end @@ -1211,7 +1211,6 @@ mutable struct UserPasswordCredential <: AbstractCredential end function shred!(cred::UserPasswordCredential) - shred!(cred.user) shred!(cred.pass) return cred end @@ -1226,12 +1225,13 @@ end "SSH credential type" mutable struct SSHCredential <: AbstractCredential - user::SecureString + user::AbstractString pass::SecureString - prvkey::SecureString - pubkey::SecureString - function SSHCredential(user::AbstractString="", pass::AbstractString="", - prvkey::AbstractString="", pubkey::AbstractString="") + # Paths to private keys + prvkey::AbstractString + pubkey::AbstractString + function SSHCredential(user="", pass="", + prvkey="", pubkey="") new(user, pass, prvkey, pubkey) end @@ -1248,10 +1248,7 @@ mutable struct SSHCredential <: AbstractCredential end function shred!(cred::SSHCredential) - shred!(cred.user) shred!(cred.pass) - shred!(cred.prvkey) - shred!(cred.pubkey) return cred end diff --git a/stdlib/LibGit2/src/utils.jl b/stdlib/LibGit2/src/utils.jl index ea60f3580e238..401e06415ce96 100644 --- a/stdlib/LibGit2/src/utils.jl +++ b/stdlib/LibGit2/src/utils.jl @@ -122,37 +122,36 @@ julia> LibGit2.git_url(scheme="ssh", username="git", host="github.com", port=222 function git_url(; scheme::AbstractString="", username::AbstractString="", - password::AbstractString="", host::AbstractString="", - port::Union{AbstractString,Integer}="", + port::Union{AbstractString, Integer}="", path::AbstractString="") - port_str = string(port) + port_str = port isa Integer ? string(port) : port scp_syntax = isempty(scheme) isempty(host) && throw(ArgumentError("A host needs to be specified")) scp_syntax && !isempty(port_str) && throw(ArgumentError("Port cannot be specified when using scp-like syntax")) io = IOBuffer() - !isempty(scheme) && print(io, scheme, "://") + !isempty(scheme) && write(io, scheme, "://") - if !isempty(username) || !isempty(password) - print(io, username) - !isempty(password) && print(io, ':', password) - print(io, '@') + if !isempty(username) + write(io, username) + write(io, '@') end - print(io, host) - !isempty(port_str) && print(io, ':', port_str) + write(io, host) + !isempty(port_str) && write(io, ':', port_str) if !isempty(path) if scp_syntax - print(io, ':') + write(io, ':') elseif !startswith(path, '/') - print(io, '/') + write(io, '/') end - print(io, path) + write(io, path) end + seek(io, 0) return String(take!(io)) end diff --git a/stdlib/LibGit2/test/libgit2.jl b/stdlib/LibGit2/test/libgit2.jl index 131cb33053b44..225ffb9f036e4 100644 --- a/stdlib/LibGit2/test/libgit2.jl +++ b/stdlib/LibGit2/test/libgit2.jl @@ -11,11 +11,15 @@ isdefined(Main, :TestHelpers) || @eval Main include(joinpath($(BASE_TEST_PATH), import .Main.TestHelpers: with_fake_pty function challenge_prompt(code::Expr, challenges; timeout::Integer=60, debug::Bool=true) + input_code = tempname() + open(input_code, "w") do fp + serialize(fp, code) + end output_file = tempname() wrapped_code = quote using Serialization - result = let - $code + result = open($input_code) do fp + eval(deserialize(fp)) end open($output_file, "w") do fp serialize(fp, result) @@ -30,6 +34,7 @@ function challenge_prompt(code::Expr, challenges; timeout::Integer=60, debug::Bo end finally isfile(output_file) && rm(output_file) + isfile(input_code) && rm(input_code) end return nothing end @@ -318,27 +323,26 @@ end url = LibGit2.git_url( scheme="https", username="user", - password="pass", + # password="pass", host="server.com", port=80, path="org/project.git") - @test url == "https://user:pass@server.com:80/org/project.git" + @test url == "https://user@server.com:80/org/project.git" end @testset "SSH URL" begin url = LibGit2.git_url( scheme="ssh", username="user", - password="pass", + # password="pass", host="server", port="22", path="project.git") - @test url == "ssh://user:pass@server:22/project.git" + @test url == "ssh://user@server:22/project.git" end @testset "SSH URL, scp-like syntax" begin url = LibGit2.git_url( - scheme="", username="user", host="server", path="project.git") @@ -365,10 +369,10 @@ end url = LibGit2.git_url( scheme="https", username="user", - password="pass", + # password="pass", host="server.com", port="80") - @test url == "https://user:pass@server.com:80" + @test url == "https://user@server.com:80" end @testset "scp-like syntax, no path" begin @@ -393,7 +397,7 @@ end url = LibGit2.git_url( scheme="", username="", - password="", + # password="", host="server.com", port="", path="") @@ -427,8 +431,9 @@ end @testset "missing" begin str = "" cred = read!(IOBuffer(str), LibGit2.GitCredential()) - @test cred == LibGit2.GitCredential() + # @test cred == LibGit2.GitCredential() @test sprint(write, cred) == str + shred!(cred) end @testset "empty" begin @@ -442,6 +447,7 @@ end cred = read!(IOBuffer(str), LibGit2.GitCredential()) @test cred == LibGit2.GitCredential("", "", "", "", "") @test sprint(write, cred) == str + shred!(cred) end @testset "input/output" begin @@ -1674,7 +1680,7 @@ mktempdir() do dir @testset "Credentials" begin creds_user = "USER" - creds_pass = "PASS" + creds_pass = SecureString("PASS") creds = LibGit2.UserPasswordCredential(creds_user, creds_pass) @test creds.user == creds_user @test creds.pass == creds_pass @@ -1684,8 +1690,8 @@ mktempdir() do dir sshcreds = LibGit2.SSHCredential(creds_user, creds_pass) @test sshcreds.user == creds_user @test sshcreds.pass == creds_pass - @test isempty(sshcreds.prvkey) - @test isempty(sshcreds.pubkey) + @test sshcreds.prvkey == "" + @test sshcreds.pubkey == "" sshcreds2 = LibGit2.SSHCredential(creds_user, creds_pass) @test sshcreds == sshcreds2 @@ -1693,6 +1699,7 @@ mktempdir() do dir shred!(creds2) shred!(sshcreds) shred!(sshcreds2) + shred!(creds_pass) end @testset "CachedCredentials" begin @@ -1703,12 +1710,13 @@ mktempdir() do dir cred = LibGit2.UserPasswordCredential("julia", "password") @test !haskey(cache, cred_id) + password = SecureString("password") # Attempt to reject a credential which wasn't stored LibGit2.reject(cache, cred, url) @test !haskey(cache, cred_id) @test cred.user == "julia" - @test cred.pass == "password" + @test cred.pass == password # Approve a credential which causes it to be stored LibGit2.approve(cache, cred, url) @@ -1719,10 +1727,11 @@ mktempdir() do dir LibGit2.reject(cache, cred, url) @test !haskey(cache, cred_id) @test cred.user == "julia" - @test cred.pass == "password" + @test cred.pass == password shred!(cache) shred!(cred) + shred!(password) end @testset "Git credential username" begin @@ -2516,7 +2525,7 @@ mktempdir() do dir @test err == exhausted_error @test auth_attempts == 3 @test p.explicit == invalid_cred - @test p.credential != invalid_cred + # @test p.credential != invalid_cred # TODO!!! shred!(valid_cred) shred!(invalid_cred) @@ -2715,7 +2724,7 @@ mktempdir() do dir @test err == incompatible_error @test auth_attempts == 1 @test p.explicit == valid_cred - @test p.credential != valid_cred + # @test p.credential != valid_cred # TODO!!! shred!(valid_cred) end @@ -2902,7 +2911,7 @@ end let cache = LibGit2.CachedCredentials() get!(cache, "foo", LibGit2.SSHCredential("", "bar")) shred!(cache) - @test cache["foo"].pass == "\0\0\0" + @test all(cache["foo"].pass.data .== UInt(0)) end end # module diff --git a/stdlib/LibGit2/test/online.jl b/stdlib/LibGit2/test/online.jl index 6de50a66a3492..46da0f80820d2 100644 --- a/stdlib/LibGit2/test/online.jl +++ b/stdlib/LibGit2/test/online.jl @@ -70,6 +70,7 @@ mktempdir() do dir @test isa(ex, LibGit2.Error.GitError) @test ex.code == LibGit2.Error.EAUTH end + shred!(cred) end @testset "Empty Credentials" begin From 16e6523992e193cc09448658cd88ed48f360edef Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Wed, 13 Jun 2018 17:15:48 -0400 Subject: [PATCH 03/19] Rename to SecretBuffer --- base/exports.jl | 2 - base/strings/secure.jl | 103 +++++++++++-------- base/util.jl | 8 +- stdlib/LibGit2/src/callbacks.jl | 2 +- stdlib/LibGit2/src/gitcredential.jl | 16 +-- stdlib/LibGit2/src/types.jl | 24 ++--- stdlib/LibGit2/test/libgit2.jl | 148 ++++++++++++++-------------- stdlib/LibGit2/test/online.jl | 2 +- 8 files changed, 161 insertions(+), 144 deletions(-) diff --git a/base/exports.jl b/base/exports.jl index d3005f29dd7e8..37da399892e43 100644 --- a/base/exports.jl +++ b/base/exports.jl @@ -75,7 +75,6 @@ export RoundNearestTiesUp, RoundToZero, RoundUp, - SecureString, Set, Some, StepRange, @@ -587,7 +586,6 @@ export rpad, rsplit, rstrip, - shred!, split, string, strip, diff --git a/base/strings/secure.jl b/base/strings/secure.jl index b4715cd18bc5d..2d8c926ba1159 100644 --- a/base/strings/secure.jl +++ b/base/strings/secure.jl @@ -1,67 +1,86 @@ """ - SecureString(string::AbstractString) + Base.SecretBuffer() -A string where the contents will be securely wiped when garbage collected. However, it is -considered best practise to wipe the string using `shred!(::SecureString)` as soon as the -secure data is no longer required. Note that when the parameter is of type `Vector{UInt8}` -then the memory of the passed in parameter will also be securely wiped. +An IOBuffer-like object where the contents will be securely wiped when garbage collected. However, it is +considered best practice to wipe the buffer using `Base.shred!(::SecretBuffer)` as soon as the +secure data are no longer required. Avoid initializing with converting to strings as they are +unable to be explicitly zeroed through mutation; when initializing with existing data the `SecretBuffer!` +function is recommended to securely zero the passed argument. # Examples ```jldoctest -julia> str = "abc"::String -"abc" +julia> s = Base.SecretBuffer() +SecretBuffer("*******") -julia> s = SecureString(str) -"abc" +julia> write(s, 's', 'e', 'c', 'r', 'e', 't') +6 -julia> shred!(s) -"\0\0\0" +julia> seek(s, 0); Char(read(s, UInt8)) +'s': ASCII/Unicode U+0073 (category Ll: Letter, lowercase) -julia> str -"abc" +julia> Base.shred!(s) +SecretBuffer("*******") + +julia> eof(s) +true ``` """ -mutable struct SecureString <: IO +mutable struct SecretBuffer <: IO data::Vector{UInt8} size::Int ptr::Int - function SecureString(; sizehint=128) + function SecretBuffer(; sizehint=128) s = new(Vector{UInt8}(undef, sizehint), 0, 1) finalizer(final_shred!, s) return s end end -function SecureString!(p::Ptr{UInt8}) - # Copy into our own Vector and zero the source +convert(::Type{SecretBuffer}, s::AbstractString) = SecretBuffer(String(s)) +SecretBuffer(str::AbstractString) = SecretBuffer(String(str)) +function SecretBuffer(str::String) + buf = unsafe_wrap(Vector{UInt8}, str) + s = SecretBuffer(sizehint=length(buf)) + for c in buf + write(s, c) + end + seek(s, 0) + s +end + +""" + SecretBuffer!(data::Union{Cstring, Ptr{UInt8}, Vector{UInt8}}) + +Initialize a new `SecretBuffer` with `data` and securely zero the original source argument. +""" +SecretBuffer!(s::Cstring) = SecretBuffer!(convert(Ptr{UInt8}, s)) +function SecretBuffer!(p::Ptr{UInt8}) len = ccall(:strlen, Cint, (Ptr{UInt8},), p) - s = SecureString(sizehint=len) + s = SecretBuffer(sizehint=len) for i in 1:len write(s, unsafe_load(p, i)) end seek(s, 0) + unsafe_securezero!(p, len) s end - -convert(::Type{SecureString}, s::AbstractString) = SecureString(String(s)) -SecureString(str::AbstractString) = SecureString(String(str)) -function SecureString(str::String) - buf = unsafe_wrap(Vector{UInt8}, str) - s = SecureString(sizehint=length(buf)) - for c in buf - write(s, c) +function SecretBuffer!(d::Vector{UInt8}) + s = SecretBuffer(sizehint=length(d)) + for i in 1:len + write(s, unsafe_load(p, i)) end seek(s, 0) + securezero!(d) s end -show(io::IO, s::SecureString) = print(io, "SecureString(\"*******\")") +show(io::IO, s::SecretBuffer) = print(io, "SecretBuffer(\"*******\")") -hash(s::SecureString, h::UInt) = hash(SecureString, hash((s.data, s.size, s.ptr), h)) -==(s1::SecureString, s2::SecureString) = (view(s1.data, 1:s1.size), s1.ptr) == (view(s2.data, 1:s2.size), s2.ptr) +hash(s::SecretBuffer, h::UInt) = hash(SecretBuffer, hash((s.data, s.size, s.ptr), h)) +==(s1::SecretBuffer, s2::SecretBuffer) = (view(s1.data, 1:s1.size), s1.ptr) == (view(s2.data, 1:s2.size), s2.ptr) -function write(io::SecureString, b::UInt8) +function write(io::SecretBuffer, b::UInt8) if io.ptr > length(io.data) # We need to resize! the array: do this manually to ensure no copies are left behind newdata = Vector{UInt8}(undef, (io.size+16)*2) @@ -75,7 +94,7 @@ function write(io::SecureString, b::UInt8) return 1 end -function write(io::IO, s::SecureString) +function write(io::IO, s::SecretBuffer) nb = 0 for i in 1:s.size nb += write(io, s.data[i]) @@ -83,7 +102,7 @@ function write(io::IO, s::SecureString) return nb end -function unsafe_convert(::Type{Ptr{UInt8}}, s::SecureString) +function unsafe_convert(::Type{Ptr{UInt8}}, s::SecretBuffer) # Add a hidden nul byte just past the end of the valid region p = s.ptr s.ptr = s.size + 1 @@ -93,36 +112,36 @@ function unsafe_convert(::Type{Ptr{UInt8}}, s::SecureString) return unsafe_convert(Ptr{UInt8}, s.data) end -seek(io::SecureString, n::Integer) = (io.ptr = max(min(n+1, io.size+1), 1)) -bytesavailable(io::SecureString) = io.size - io.ptr + 1 -position(io::SecureString) = io.ptr-1 -eof(io::SecureString) = io.ptr > io.size -isempty(io::SecureString) = io.size == 0 -function peek(io::SecureString) +seek(io::SecretBuffer, n::Integer) = (io.ptr = max(min(n+1, io.size+1), 1)) +bytesavailable(io::SecretBuffer) = io.size - io.ptr + 1 +position(io::SecretBuffer) = io.ptr-1 +eof(io::SecretBuffer) = io.ptr > io.size +isempty(io::SecretBuffer) = io.size == 0 +function peek(io::SecretBuffer) eof(io) && throw(EOFError()) return io.data[io.ptr] end -function read(io::SecureString, ::Type{UInt8}) +function read(io::SecretBuffer, ::Type{UInt8}) eof(io) && throw(EOFError()) byte = io.data[io.ptr] io.ptr += 1 return byte end -function final_shred!(s::SecureString) +function final_shred!(s::SecretBuffer) if !isshredded(s) shred!(s) end end -function shred!(s::SecureString) +function shred!(s::SecretBuffer) securezero!(s.data) s.ptr = 1 s.size = 0 return s end -isshredded(s::SecureString) = all(iszero, s.data) +isshredded(s::SecretBuffer) = all(iszero, s.data) function shred!(f::Function, x) try diff --git a/base/util.jl b/base/util.jl index f1ec3a2b2e9b5..4027e122d3318 100644 --- a/base/util.jl +++ b/base/util.jl @@ -441,7 +441,7 @@ will always be called. """ function securezero! end @noinline securezero!(a::AbstractArray{<:Number}) = fill!(a, 0) -securezero!(s::Union{String,Cstring}) = unsafe_securezero!(pointer(s), sizeof(s)) +securezero!(s::Cstring) = unsafe_securezero!(pointer(s), sizeof(s)) @noinline unsafe_securezero!(p::Ptr{T}, len::Integer=1) where {T} = ccall(:memset, Ptr{T}, (Ptr{T}, Cint, Csize_t), p, 0, len*sizeof(T)) unsafe_securezero!(p::Ptr{Cvoid}, len::Integer=1) = Ptr{Cvoid}(unsafe_securezero!(Ptr{UInt8}(p), len)) @@ -450,7 +450,7 @@ if Sys.iswindows() function getpass(prompt::AbstractString) print(prompt) flush(stdout) - s = SecureString() + s = SecretBuffer() plen = 0 while true c = ccall(:_getch, UInt8, ()) @@ -468,7 +468,7 @@ function getpass(prompt::AbstractString) end else function getpass(prompt::AbstractString) - SecureString(ccall(:getpass, Cstring, (Cstring,), prompt)) + SecretBuffer!(ccall(:getpass, Cstring, (Cstring,), prompt)) end end @@ -577,7 +577,7 @@ if Sys.iswindows() # Done. passbuf_ = passbuf[1:passlen[]-1] result = (String(transcode(UInt8, usernamebuf[1:usernamelen[]-1])), - SecureString(transcode(UInt8, passbuf_))) + SecretBuffer!(transcode(UInt8, passbuf_))) securezero!(passbuf_) securezero!(passbuf) diff --git a/stdlib/LibGit2/src/callbacks.jl b/stdlib/LibGit2/src/callbacks.jl index 88e8ccf4df5a2..cbd11a826275f 100644 --- a/stdlib/LibGit2/src/callbacks.jl +++ b/stdlib/LibGit2/src/callbacks.jl @@ -189,7 +189,7 @@ function authenticate_userpass(libgit2credptr::Ptr{Ptr{Cvoid}}, p::CredentialPay cred.user = something(git_cred.username, "") cred.pass = something(git_cred.password, "") - shred!(git_cred) + Base.shred!(git_cred) revised = true p.use_git_helpers = false diff --git a/stdlib/LibGit2/src/gitcredential.jl b/stdlib/LibGit2/src/gitcredential.jl index ccb21aa757d44..bb8288bea25c6 100644 --- a/stdlib/LibGit2/src/gitcredential.jl +++ b/stdlib/LibGit2/src/gitcredential.jl @@ -11,7 +11,7 @@ mutable struct GitCredential host::Union{AbstractString, Nothing} path::Union{AbstractString, Nothing} username::Union{AbstractString, Nothing} - password::Union{SecureString, Nothing} + password::Union{Base.SecretBuffer, Nothing} use_http_path::Bool function GitCredential( @@ -34,12 +34,12 @@ Base.:(==)(c1::GitCredential, c2::GitCredential) = (c1.protocol, c1.host, c1.pat (c2.protocol, c2.host, c2.path, c2.username, c2.password, c2.use_http_path) Base.hash(cred::GitCredential, h::UInt) = hash(GitCredential, hash((cred.protocol, cred.host, cred.path, cred.username, cred.password, cred.use_http_path), h)) -function shred!(cred::GitCredential) +function Base.shred!(cred::GitCredential) cred.protocol !== nothing && (cred.protocol = nothing) cred.host !== nothing && (cred.host = nothing) cred.path !== nothing && (cred.path = nothing) cred.username !== nothing && (cred.username = nothing) - cred.password !== nothing && shred!(cred.password) + cred.password !== nothing && Base.shred!(cred.password) return cred end @@ -101,7 +101,7 @@ function Base.read!(io::IO, cred::GitCredential) while !eof(io) key = readuntil(io, '=') if key == "password" - value = SecureString() + value = Base.SecretBuffer() while !eof(io) && (c = read(io, UInt8)) != UInt8('\n') write(value, c) end @@ -112,11 +112,11 @@ function Base.read!(io::IO, cred::GitCredential) if key == "url" # Any components which are missing from the URL will be set to empty # https://git-scm.com/docs/git-credential#git-credential-codeurlcode - shred!(cred) + Base.shred!(cred) copy!(cred, parse(GitCredential, value)) else field = getproperty(cred, Symbol(key)) - field !== nothing && Symbol(key) == :password && shred!(field) + field !== nothing && Symbol(key) == :password && Base.shred!(field) setproperty!(cred, Symbol(key), value) end end @@ -282,7 +282,7 @@ function approve(cfg::GitConfig, cred::UserPasswordCredential, url::AbstractStri approve(helper, git_cred) end - shred!(git_cred) + Base.shred!(git_cred) nothing end @@ -294,6 +294,6 @@ function reject(cfg::GitConfig, cred::UserPasswordCredential, url::AbstractStrin reject(helper, git_cred) end - shred!(git_cred) + Base.shred!(git_cred) nothing end diff --git a/stdlib/LibGit2/src/types.jl b/stdlib/LibGit2/src/types.jl index 4d0b182a2fc32..3c1020854cf42 100644 --- a/stdlib/LibGit2/src/types.jl +++ b/stdlib/LibGit2/src/types.jl @@ -1180,7 +1180,7 @@ function objtype(obj_type::Consts.OBJECT) end end -import Base.shred! +import Base.Base.shred! abstract type AbstractCredential end @@ -1194,8 +1194,8 @@ isfilled(::AbstractCredential) "Credential that support only `user` and `password` parameters" mutable struct UserPasswordCredential <: AbstractCredential user::AbstractString - pass::SecureString - function UserPasswordCredential(user::AbstractString="", pass::Union{AbstractString, SecureString}="") + pass::Base.SecretBuffer + function UserPasswordCredential(user::AbstractString="", pass::Union{AbstractString, Base.SecretBuffer}="") new(user, pass) end @@ -1210,8 +1210,8 @@ mutable struct UserPasswordCredential <: AbstractCredential UserPasswordCredential(prompt_if_incorrect::Bool) = UserPasswordCredential("","",prompt_if_incorrect) end -function shred!(cred::UserPasswordCredential) - shred!(cred.pass) +function Base.shred!(cred::UserPasswordCredential) + Base.shred!(cred.pass) return cred end @@ -1226,7 +1226,7 @@ end "SSH credential type" mutable struct SSHCredential <: AbstractCredential user::AbstractString - pass::SecureString + pass::Base.SecretBuffer # Paths to private keys prvkey::AbstractString pubkey::AbstractString @@ -1247,8 +1247,8 @@ mutable struct SSHCredential <: AbstractCredential SSHCredential(prompt_if_incorrect::Bool) = SSHCredential("","","","",prompt_if_incorrect) end -function shred!(cred::SSHCredential) - shred!(cred.pass) +function Base.shred!(cred::SSHCredential) + Base.shred!(cred.pass) return cred end @@ -1271,8 +1271,8 @@ Base.haskey(cache::CachedCredentials, cred_id) = Base.haskey(cache.cred, cred_id Base.getindex(cache::CachedCredentials, cred_id) = Base.getindex(cache.cred, cred_id) Base.get!(cache::CachedCredentials, cred_id, default) = Base.get!(cache.cred, cred_id, default) -function shred!(p::CachedCredentials) - foreach(shred!, values(p.cred)) +function Base.shred!(p::CachedCredentials) + foreach(Base.shred!, values(p.cred)) return p end @@ -1386,7 +1386,7 @@ function approve(p::CredentialPayload; shred::Bool=true) approve(p.config, cred, p.url) end - shred && shred!(cred) + shred && Base.shred!(cred) nothing end @@ -1411,7 +1411,7 @@ function reject(p::CredentialPayload; shred::Bool=true) reject(p.config, cred, p.url) end - shred && shred!(cred) + shred && Base.shred!(cred) nothing end diff --git a/stdlib/LibGit2/test/libgit2.jl b/stdlib/LibGit2/test/libgit2.jl index 225ffb9f036e4..23338fefe9e86 100644 --- a/stdlib/LibGit2/test/libgit2.jl +++ b/stdlib/LibGit2/test/libgit2.jl @@ -433,7 +433,7 @@ end cred = read!(IOBuffer(str), LibGit2.GitCredential()) # @test cred == LibGit2.GitCredential() @test sprint(write, cred) == str - shred!(cred) + Base.shred!(cred) end @testset "empty" begin @@ -447,7 +447,7 @@ end cred = read!(IOBuffer(str), LibGit2.GitCredential()) @test cred == LibGit2.GitCredential("", "", "", "", "") @test sprint(write, cred) == str - shred!(cred) + Base.shred!(cred) end @testset "input/output" begin @@ -462,8 +462,8 @@ end cred = read!(IOBuffer(str), LibGit2.GitCredential()) @test cred == expected_cred @test sprint(write, cred) == str - shred!(cred) - shred!(expected_cred) + Base.shred!(cred) + Base.shred!(expected_cred) end @testset "use http path" begin @@ -480,7 +480,7 @@ end @test cred.path == "dir/file" @test sprint(write, cred) == expected - shred!(cred) + Base.shred!(cred) end @testset "URL input/output" begin @@ -501,39 +501,39 @@ end cred = read!(IOBuffer(str), LibGit2.GitCredential()) @test cred == expected_cred @test sprint(write, cred) == expected_str - shred!(cred) - shred!(expected_cred) + Base.shred!(cred) + Base.shred!(expected_cred) end @testset "ismatch" begin # Equal cred = LibGit2.GitCredential("https", "github.com") @test LibGit2.ismatch("https://github.com", cred) - shred!(cred) + Base.shred!(cred) # Credential hostname is different cred = LibGit2.GitCredential("https", "github.com") @test !LibGit2.ismatch("https://myhost", cred) - shred!(cred) + Base.shred!(cred) # Credential is less specific than URL cred = LibGit2.GitCredential("https") @test !LibGit2.ismatch("https://github.com", cred) - shred!(cred) + Base.shred!(cred) # Credential is more specific than URL cred = LibGit2.GitCredential("https", "github.com", "path", "user", "pass") @test LibGit2.ismatch("https://github.com", cred) - shred!(cred) + Base.shred!(cred) # Credential needs to have an "" username to match cred = LibGit2.GitCredential("https", "github.com", nothing, "") @test LibGit2.ismatch("https://@github.com", cred) - shred!(cred) + Base.shred!(cred) cred = LibGit2.GitCredential("https", "github.com", nothing, nothing) @test !LibGit2.ismatch("https://@github.com", cred) - shred!(cred) + Base.shred!(cred) end end @@ -1680,7 +1680,7 @@ mktempdir() do dir @testset "Credentials" begin creds_user = "USER" - creds_pass = SecureString("PASS") + creds_pass = Base.SecretBuffer("PASS") creds = LibGit2.UserPasswordCredential(creds_user, creds_pass) @test creds.user == creds_user @test creds.pass == creds_pass @@ -1695,11 +1695,11 @@ mktempdir() do dir sshcreds2 = LibGit2.SSHCredential(creds_user, creds_pass) @test sshcreds == sshcreds2 - shred!(creds) - shred!(creds2) - shred!(sshcreds) - shred!(sshcreds2) - shred!(creds_pass) + Base.shred!(creds) + Base.shred!(creds2) + Base.shred!(sshcreds) + Base.shred!(sshcreds2) + Base.shred!(creds_pass) end @testset "CachedCredentials" begin @@ -1710,7 +1710,7 @@ mktempdir() do dir cred = LibGit2.UserPasswordCredential("julia", "password") @test !haskey(cache, cred_id) - password = SecureString("password") + password = Base.SecretBuffer("password") # Attempt to reject a credential which wasn't stored LibGit2.reject(cache, cred, url) @@ -1729,9 +1729,9 @@ mktempdir() do dir @test cred.user == "julia" @test cred.pass == password - shred!(cache) - shred!(cred) - shred!(password) + Base.shred!(cache) + Base.shred!(cred) + Base.shred!(password) end @testset "Git credential username" begin @@ -1769,8 +1769,8 @@ mktempdir() do dir username = LibGit2.default_username(cfg, mygit_cred) @test username == "bar" - shred!(github_cred) - shred!(mygit_cred) + Base.shred!(github_cred) + Base.shred!(mygit_cred) end end @@ -1795,8 +1795,8 @@ mktempdir() do dir username = LibGit2.default_username(cfg, mygit_cred) @test username == "name" - shred!(github_cred) - shred!(mygit_cred) + Base.shred!(github_cred) + Base.shred!(mygit_cred) end end end @@ -1830,8 +1830,8 @@ mktempdir() do dir @test !LibGit2.use_http_path(cfg, github_cred) @test LibGit2.use_http_path(cfg, mygit_cred) - shred!(github_cred) - shred!(mygit_cred) + Base.shred!(github_cred) + Base.shred!(mygit_cred) end end end @@ -1877,8 +1877,8 @@ mktempdir() do dir println(stderr, "The following 'Resetting the helper list...' warning is expected:") @test_broken LibGit2.credential_helpers(cfg, mygit_cred) == expected[2] - shred!(github_cred) - shred!(mygit_cred) + Base.shred!(github_cred) + Base.shred!(mygit_cred) end end @@ -1901,23 +1901,23 @@ mktempdir() do dir @test !isfile(credential_path) - shred!(LibGit2.fill!(helper, deepcopy(query))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query))) do result @test result == query end LibGit2.approve(helper, filled) @test isfile(credential_path) - shred!(LibGit2.fill!(helper, deepcopy(query))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query))) do result @test result == filled end LibGit2.reject(helper, filled) - shred!(LibGit2.fill!(helper, deepcopy(query))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query))) do result @test result == query end - shred!(query) - shred!(filled) + Base.shred!(query) + Base.shred!(filled) end end end @@ -1954,57 +1954,57 @@ mktempdir() do dir @test !isfile(credential_path) - shred!(LibGit2.fill!(helper, deepcopy(query))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query))) do result @test result == query end - shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result @test result == query_a end - shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result @test result == query_b end LibGit2.approve(helper, filled_a) @test isfile(credential_path) - shred!(LibGit2.fill!(helper, deepcopy(query))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query))) do result @test result == filled_without_path_a end - shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result @test result == filled_a end - shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result @test result == query_b end LibGit2.approve(helper, filled_b) - shred!(LibGit2.fill!(helper, deepcopy(query))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query))) do result @test result == filled_without_path_b end - shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result @test result == filled_a end - shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result @test result == filled_b end LibGit2.reject(helper, filled_b) - shred!(LibGit2.fill!(helper, deepcopy(query))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query))) do result @test result == filled_without_path_a end - shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query_a))) do result @test result == filled_a end - shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result + Base.shred!(LibGit2.fill!(helper, deepcopy(query_b))) do result @test result == query_b end - shred!(query) - shred!(query_a) - shred!(query_b) - shred!(filled_a) - shred!(filled_b) - shred!(filled_without_path_a) - shred!(filled_without_path_b) + Base.shred!(query) + Base.shred!(query_a) + Base.shred!(query_b) + Base.shred!(filled_a) + Base.shred!(filled_b) + Base.shred!(filled_without_path_a) + Base.shred!(filled_without_path_b) end end end @@ -2276,8 +2276,8 @@ mktempdir() do dir @test auth_attempts == 2 end - shred!(valid_cred) - shred!(valid_p_cred) + Base.shred!(valid_cred) + Base.shred!(valid_p_cred) end @testset "HTTPS credential prompt" begin @@ -2342,7 +2342,7 @@ mktempdir() do dir @test err == prompt_limit @test auth_attempts == 3 - shred!(valid_cred) + Base.shred!(valid_cred) end @testset "SSH agent username" begin @@ -2373,7 +2373,7 @@ mktempdir() do dir @test err == exhausted_error @test auth_attempts == 2 - shred!(valid_cred) + Base.shred!(valid_cred) end @testset "SSH default" begin @@ -2435,8 +2435,8 @@ mktempdir() do dir @test auth_attempts == 1 end - shred!(valid_cred) - shred!(valid_p_cred) + Base.shred!(valid_cred) + Base.shred!(valid_p_cred) end end @@ -2486,7 +2486,7 @@ mktempdir() do dir @test p.credential.pubkey == abspath(valid_key * ".pub") end - shred!(valid_cred) + Base.shred!(valid_cred) end @testset "SSH explicit credentials" begin @@ -2527,8 +2527,8 @@ mktempdir() do dir @test p.explicit == invalid_cred # @test p.credential != invalid_cred # TODO!!! - shred!(valid_cred) - shred!(invalid_cred) + Base.shred!(valid_cred) + Base.shred!(invalid_cred) end @testset "HTTPS explicit credentials" begin @@ -2562,8 +2562,8 @@ mktempdir() do dir @test p.explicit == invalid_cred @test p.credential != invalid_cred - shred!(valid_cred) - shred!(invalid_cred) + Base.shred!(valid_cred) + Base.shred!(invalid_cred) end @testset "Cached credentials" begin @@ -2649,8 +2649,8 @@ mktempdir() do dir @test cache.cred == Dict() @test p.credential != invalid_cred - shred!(valid_cred) - shred!(invalid_cred) + Base.shred!(valid_cred) + Base.shred!(invalid_cred) end @testset "HTTPS git helper username" begin @@ -2688,7 +2688,7 @@ mktempdir() do dir # Verify credential wasn't accidentally zeroed (#24731) @test p.credential == valid_cred - shred!(valid_cred) + Base.shred!(valid_cred) end @testset "Incompatible explicit credentials" begin @@ -2708,7 +2708,7 @@ mktempdir() do dir @test p.explicit == valid_cred @test p.credential != valid_cred - shred!(valid_cred) + Base.shred!(valid_cred) # User provides a SSH credential where a user/password credential is required. valid_cred = LibGit2.SSHCredential("foo", "", "", "") @@ -2726,7 +2726,7 @@ mktempdir() do dir @test p.explicit == valid_cred # @test p.credential != valid_cred # TODO!!! - shred!(valid_cred) + Base.shred!(valid_cred) end # A hypothetical scenario where the the allowed authentication can either be @@ -2749,7 +2749,7 @@ mktempdir() do dir @test err == git_ok @test auth_attempts == 1 - shred!(valid_cred) + Base.shred!(valid_cred) end @testset "CredentialPayload reset" begin @@ -2789,7 +2789,7 @@ mktempdir() do dir @test err == git_ok @test auth_attempts == 1 - shred!(valid_cred) + Base.shred!(valid_cred) end end @@ -2910,7 +2910,7 @@ end let cache = LibGit2.CachedCredentials() get!(cache, "foo", LibGit2.SSHCredential("", "bar")) - shred!(cache) + Base.shred!(cache) @test all(cache["foo"].pass.data .== UInt(0)) end diff --git a/stdlib/LibGit2/test/online.jl b/stdlib/LibGit2/test/online.jl index 46da0f80820d2..7c7029d3f064c 100644 --- a/stdlib/LibGit2/test/online.jl +++ b/stdlib/LibGit2/test/online.jl @@ -70,7 +70,7 @@ mktempdir() do dir @test isa(ex, LibGit2.Error.GitError) @test ex.code == LibGit2.Error.EAUTH end - shred!(cred) + Base.shred!(cred) end @testset "Empty Credentials" begin From e35a63539b916ec13b415e4da581b781eb0ef69e Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Wed, 13 Jun 2018 17:28:57 -0400 Subject: [PATCH 04/19] Rename file to reflect new name --- base/{strings/secure.jl => secretbuffer.jl} | 0 base/strings/strings.jl | 1 - base/sysimg.jl | 1 + 3 files changed, 1 insertion(+), 1 deletion(-) rename base/{strings/secure.jl => secretbuffer.jl} (100%) diff --git a/base/strings/secure.jl b/base/secretbuffer.jl similarity index 100% rename from base/strings/secure.jl rename to base/secretbuffer.jl diff --git a/base/strings/strings.jl b/base/strings/strings.jl index 6a6eeaa703a03..0da4b6312c8ce 100644 --- a/base/strings/strings.jl +++ b/base/strings/strings.jl @@ -9,4 +9,3 @@ import .Unicode: textwidth, islowercase, isuppercase, isletter, isdigit, isnumer include("strings/util.jl") include("strings/io.jl") -include("strings/secure.jl") diff --git a/base/sysimg.jl b/base/sysimg.jl index 7c22e97db2bfd..2f6305aecb454 100644 --- a/base/sysimg.jl +++ b/base/sysimg.jl @@ -246,6 +246,7 @@ include("c.jl") include("io.jl") include("iostream.jl") include("iobuffer.jl") +include("secretbuffer.jl") # strings & printing include("intfuncs.jl") From cafe85053b9299bad24963cc61a5d089f6062cce Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Thu, 14 Jun 2018 13:25:56 -0400 Subject: [PATCH 05/19] Updates from review --- base/secretbuffer.jl | 35 ++++++++++++++++------------- base/util.jl | 9 ++++---- stdlib/LibGit2/src/gitcredential.jl | 27 ++++++++++++---------- stdlib/LibGit2/src/types.jl | 14 +++++++----- stdlib/LibGit2/src/utils.jl | 2 +- stdlib/LibGit2/test/libgit2.jl | 10 +++------ stdlib/OldPkg/src/entry.jl | 5 +---- 7 files changed, 52 insertions(+), 50 deletions(-) diff --git a/base/secretbuffer.jl b/base/secretbuffer.jl index 2d8c926ba1159..c3d00093ceee6 100644 --- a/base/secretbuffer.jl +++ b/base/secretbuffer.jl @@ -1,11 +1,12 @@ """ Base.SecretBuffer() -An IOBuffer-like object where the contents will be securely wiped when garbage collected. However, it is -considered best practice to wipe the buffer using `Base.shred!(::SecretBuffer)` as soon as the -secure data are no longer required. Avoid initializing with converting to strings as they are -unable to be explicitly zeroed through mutation; when initializing with existing data the `SecretBuffer!` -function is recommended to securely zero the passed argument. +An IOBuffer-like object where the contents will be securely wiped when garbage collected. + +It is considered best practice to wipe the buffer using `Base.shred!(::SecretBuffer)` as +soon as the secure data are no longer required. When initializing with existing data, the +`SecretBuffer!` method is highly recommended to securely zero the passed argument. Avoid +initializing with and converting to `String`s as they are unable to be securely zeroed. # Examples ```jldoctest @@ -40,7 +41,7 @@ end convert(::Type{SecretBuffer}, s::AbstractString) = SecretBuffer(String(s)) SecretBuffer(str::AbstractString) = SecretBuffer(String(str)) function SecretBuffer(str::String) - buf = unsafe_wrap(Vector{UInt8}, str) + buf = codepoints(str) s = SecretBuffer(sizehint=length(buf)) for c in buf write(s, c) @@ -66,9 +67,10 @@ function SecretBuffer!(p::Ptr{UInt8}) s end function SecretBuffer!(d::Vector{UInt8}) - s = SecretBuffer(sizehint=length(d)) + len = length(d) + s = SecretBuffer(sizehint=len) for i in 1:len - write(s, unsafe_load(p, i)) + write(s, d[i]) end seek(s, 0) securezero!(d) @@ -102,7 +104,11 @@ function write(io::IO, s::SecretBuffer) return nb end -function unsafe_convert(::Type{Ptr{UInt8}}, s::SecretBuffer) +function unsafe_convert(::Type{Cstring}, s::SecretBuffer) + # Ensure that no nuls appear in the valid region + if any(!(==(0x00)), s.data[i] for i in 1:s.size) + error("`SecretBuffers` containing nul bytes cannot be converted to `Cstring`") + end # Add a hidden nul byte just past the end of the valid region p = s.ptr s.ptr = s.size + 1 @@ -112,7 +118,10 @@ function unsafe_convert(::Type{Ptr{UInt8}}, s::SecretBuffer) return unsafe_convert(Ptr{UInt8}, s.data) end -seek(io::SecretBuffer, n::Integer) = (io.ptr = max(min(n+1, io.size+1), 1)) +seek(io::SecretBuffer, n::Integer) = (io.ptr = max(min(n+1, io.size+1), 1); io) +seekend(io::SecretBuffer) = seek(io, io.size+1) +skip(io::SecretBuffer, n::Integer) = seek(io, position(io) + n) + bytesavailable(io::SecretBuffer) = io.size - io.ptr + 1 position(io::SecretBuffer) = io.ptr-1 eof(io::SecretBuffer) = io.ptr > io.size @@ -128,11 +137,7 @@ function read(io::SecretBuffer, ::Type{UInt8}) return byte end -function final_shred!(s::SecretBuffer) - if !isshredded(s) - shred!(s) - end -end +final_shred!(s::SecretBuffer) = shred!(s) function shred!(s::SecretBuffer) securezero!(s.data) diff --git a/base/util.jl b/base/util.jl index 4027e122d3318..819981c27d269 100644 --- a/base/util.jl +++ b/base/util.jl @@ -441,7 +441,6 @@ will always be called. """ function securezero! end @noinline securezero!(a::AbstractArray{<:Number}) = fill!(a, 0) -securezero!(s::Cstring) = unsafe_securezero!(pointer(s), sizeof(s)) @noinline unsafe_securezero!(p::Ptr{T}, len::Integer=1) where {T} = ccall(:memset, Ptr{T}, (Ptr{T}, Cint, Csize_t), p, 0, len*sizeof(T)) unsafe_securezero!(p::Ptr{Cvoid}, len::Integer=1) = Ptr{Cvoid}(unsafe_securezero!(Ptr{UInt8}(p), len)) @@ -453,11 +452,11 @@ function getpass(prompt::AbstractString) s = SecretBuffer() plen = 0 while true - c = ccall(:_getch, UInt8, ()) + c = UInt8(ccall(:_getch, Cint, ())) if c == 0xff || c == UInt8('\n') || c == UInt8('\r') break # EOF or return elseif c == 0x00 || c == 0xe0 - ccall(:_getch, UInt8, ()) # ignore function/arrow keys + ccall(:_getch, Cint, ()) # ignore function/arrow keys elseif c == UInt8('\b') && plen > 0 plen -= 1 # delete last character on backspace elseif !iscntrl(Char(c)) && plen < 128 @@ -473,13 +472,13 @@ end end """ - prompt(message; default="", password=false) -> Union{AbstractString, Nothing} + prompt(message; default="", password=false) -> Union{String, SecretBuffer, Nothing} Displays the `message` then waits for user input. Input is terminated when a newline (\\n) is encountered or EOF (^D) character is entered on a blank line. If a `default` is provided then the user can enter just a newline character to select the `default`. Alternatively, when the `password` keyword is `true` the characters entered by the user will not be -displayed. +displayed and it will return a `SecretBuffer` instead of a `String`. """ function prompt(message::AbstractString; default::AbstractString="", password::Bool=false) if Sys.iswindows() && password diff --git a/stdlib/LibGit2/src/gitcredential.jl b/stdlib/LibGit2/src/gitcredential.jl index bb8288bea25c6..9a60e65a86b0b 100644 --- a/stdlib/LibGit2/src/gitcredential.jl +++ b/stdlib/LibGit2/src/gitcredential.jl @@ -7,10 +7,10 @@ Git credential information used in communication with git credential helpers. Th named using the [input/output key specification](https://git-scm.com/docs/git-credential#IOFMT). """ mutable struct GitCredential - protocol::Union{AbstractString, Nothing} - host::Union{AbstractString, Nothing} - path::Union{AbstractString, Nothing} - username::Union{AbstractString, Nothing} + protocol::Union{String, Nothing} + host::Union{String, Nothing} + path::Union{String, Nothing} + username::Union{String, Nothing} password::Union{Base.SecretBuffer, Nothing} use_http_path::Bool @@ -35,11 +35,12 @@ Base.:(==)(c1::GitCredential, c2::GitCredential) = (c1.protocol, c1.host, c1.pat Base.hash(cred::GitCredential, h::UInt) = hash(GitCredential, hash((cred.protocol, cred.host, cred.path, cred.username, cred.password, cred.use_http_path), h)) function Base.shred!(cred::GitCredential) - cred.protocol !== nothing && (cred.protocol = nothing) - cred.host !== nothing && (cred.host = nothing) - cred.path !== nothing && (cred.path = nothing) - cred.username !== nothing && (cred.username = nothing) + cred.protocol = nothing + cred.host = nothing + cred.path = nothing + cred.username = nothing cred.password !== nothing && Base.shred!(cred.password) + cred.password = nothing return cred end @@ -79,11 +80,12 @@ function Base.parse(::Type{GitCredential}, url::AbstractString) end function Base.copy!(a::GitCredential, b::GitCredential) + shred!(a) a.protocol = b.protocol a.host = b.host a.path = b.path a.username = b.username - a.password = b.password + a.password = copy(b.password) return a end @@ -105,15 +107,16 @@ function Base.read!(io::IO, cred::GitCredential) while !eof(io) && (c = read(io, UInt8)) != UInt8('\n') write(value, c) end - seek(value, 0) + seekstart(value) else value = readuntil(io, '\n') end if key == "url" # Any components which are missing from the URL will be set to empty # https://git-scm.com/docs/git-credential#git-credential-codeurlcode - Base.shred!(cred) - copy!(cred, parse(GitCredential, value)) + shred!(parse(GitCredential, value)) do urlcred + copy!(cred, urlcred) + end else field = getproperty(cred, Symbol(key)) field !== nothing && Symbol(key) == :password && Base.shred!(field) diff --git a/stdlib/LibGit2/src/types.jl b/stdlib/LibGit2/src/types.jl index 3c1020854cf42..cdd862f1f1d07 100644 --- a/stdlib/LibGit2/src/types.jl +++ b/stdlib/LibGit2/src/types.jl @@ -1180,8 +1180,6 @@ function objtype(obj_type::Consts.OBJECT) end end -import Base.Base.shred! - abstract type AbstractCredential end """ @@ -1193,7 +1191,7 @@ isfilled(::AbstractCredential) "Credential that support only `user` and `password` parameters" mutable struct UserPasswordCredential <: AbstractCredential - user::AbstractString + user::String pass::Base.SecretBuffer function UserPasswordCredential(user::AbstractString="", pass::Union{AbstractString, Base.SecretBuffer}="") new(user, pass) @@ -1211,6 +1209,7 @@ mutable struct UserPasswordCredential <: AbstractCredential end function Base.shred!(cred::UserPasswordCredential) + cred.user = "" Base.shred!(cred.pass) return cred end @@ -1225,11 +1224,11 @@ end "SSH credential type" mutable struct SSHCredential <: AbstractCredential - user::AbstractString + user::String pass::Base.SecretBuffer # Paths to private keys - prvkey::AbstractString - pubkey::AbstractString + prvkey::String + pubkey::String function SSHCredential(user="", pass="", prvkey="", pubkey="") new(user, pass, prvkey, pubkey) @@ -1248,7 +1247,10 @@ mutable struct SSHCredential <: AbstractCredential end function Base.shred!(cred::SSHCredential) + cred.user = "" Base.shred!(cred.pass) + cred.prvkey = "" + cred.pubkey = "" return cred end diff --git a/stdlib/LibGit2/src/utils.jl b/stdlib/LibGit2/src/utils.jl index 401e06415ce96..dd603b8e2140f 100644 --- a/stdlib/LibGit2/src/utils.jl +++ b/stdlib/LibGit2/src/utils.jl @@ -151,7 +151,7 @@ function git_url(; end write(io, path) end - seek(io, 0) + seekstart(io) return String(take!(io)) end diff --git a/stdlib/LibGit2/test/libgit2.jl b/stdlib/LibGit2/test/libgit2.jl index 23338fefe9e86..4910ba60f64d6 100644 --- a/stdlib/LibGit2/test/libgit2.jl +++ b/stdlib/LibGit2/test/libgit2.jl @@ -323,7 +323,6 @@ end url = LibGit2.git_url( scheme="https", username="user", - # password="pass", host="server.com", port=80, path="org/project.git") @@ -334,7 +333,6 @@ end url = LibGit2.git_url( scheme="ssh", username="user", - # password="pass", host="server", port="22", path="project.git") @@ -369,7 +367,6 @@ end url = LibGit2.git_url( scheme="https", username="user", - # password="pass", host="server.com", port="80") @test url == "https://user@server.com:80" @@ -397,7 +394,6 @@ end url = LibGit2.git_url( scheme="", username="", - # password="", host="server.com", port="", path="") @@ -431,7 +427,7 @@ end @testset "missing" begin str = "" cred = read!(IOBuffer(str), LibGit2.GitCredential()) - # @test cred == LibGit2.GitCredential() + @test cred == LibGit2.GitCredential() @test sprint(write, cred) == str Base.shred!(cred) end @@ -2525,7 +2521,7 @@ mktempdir() do dir @test err == exhausted_error @test auth_attempts == 3 @test p.explicit == invalid_cred - # @test p.credential != invalid_cred # TODO!!! + @test p.credential != invalid_cred Base.shred!(valid_cred) Base.shred!(invalid_cred) @@ -2724,7 +2720,7 @@ mktempdir() do dir @test err == incompatible_error @test auth_attempts == 1 @test p.explicit == valid_cred - # @test p.credential != valid_cred # TODO!!! + @test p.credential != valid_cred Base.shred!(valid_cred) end diff --git a/stdlib/OldPkg/src/entry.jl b/stdlib/OldPkg/src/entry.jl index 6c9e3faad1a41..b4a9f56025a58 100644 --- a/stdlib/OldPkg/src/entry.jl +++ b/stdlib/OldPkg/src/entry.jl @@ -420,8 +420,7 @@ function update(branch::AbstractString, upkgs::Set{String}) end end fixed = Read.fixed(avail,instd,dont_update) - creds = LibGit2.CachedCredentials() - try + shred!(LibGit2.CachedCredentials()) do creds stopupdate = false for (pkg,ver) in fixed ispath(pkg,".git") || continue @@ -463,8 +462,6 @@ function update(branch::AbstractString, upkgs::Set{String}) end end end - finally - Base.securezero!(creds) end @info "Computing changes..." resolve(reqs, avail, instd, fixed, free, upkgs) From bcc0a7b0c80b9d9cf6b74bb41a341713b3b425c0 Mon Sep 17 00:00:00 2001 From: Curtis Vogt Date: Thu, 14 Jun 2018 13:40:54 -0400 Subject: [PATCH 06/19] Add SecretBuffer testset --- test/secretbuffer.jl | 28 ++++++++++++++++++++++++++ test/strings/secure.jl | 45 ------------------------------------------ 2 files changed, 28 insertions(+), 45 deletions(-) create mode 100644 test/secretbuffer.jl delete mode 100644 test/strings/secure.jl diff --git a/test/secretbuffer.jl b/test/secretbuffer.jl new file mode 100644 index 0000000000000..9245d5766e328 --- /dev/null +++ b/test/secretbuffer.jl @@ -0,0 +1,28 @@ +using Base: SecretBuffer, SecretBuffer!, shred!, isshredded +using Base.Test + +@testset "SecretBuffer" begin + @testset "original unmodified" begin + str = "foobar" + secret = SecretBuffer(str) + + @test read(secret, String) == str + seekstart(secret) + + @test shred!(secret) === secret + @test read(secret, String) != "" + @test str != "foobar" + end + + @testset "finalizer" begin + v = UInt8[1, 2] + secret_a = SecretBuffer!(v) + secret_b = secret_a + + secret_a = nothing + GC.gc() + + @test all(iszero, v) + @test !isshredded(secret_b) + end +end diff --git a/test/strings/secure.jl b/test/strings/secure.jl deleted file mode 100644 index fcea253605dbb..0000000000000 --- a/test/strings/secure.jl +++ /dev/null @@ -1,45 +0,0 @@ -## Secure strings ## - -@testset "SecureString" begin - @testset "wipe original" begin - str = String("foobar") - secure = SecureString(str) - @test str == secure - - # Securely wiping the SecureString will also wipe out the original source - shred!(secure) === secure - @test secure != "foobar" - @test str != "foobar" - end - - @testset "finalizer" begin - # Note: Using `@test_warn` ends up messing up the test - finalizer_test() = begin - str = "foobar" - expected = deepcopy(str) - secure_a = SecureString(str) - secure_b = secure_a - - secure_a = nothing - gc() - - @test str == expected # Still retaining a reference to the original SecureString - - secure_b = nothing - gc() - - @test str != expected - end - - @test_warn "SecureString \"foobar\" not explicitly shredded" finalizer_test() - end - - @testset "deepcopy" begin - secure_a = SecureString("foo") - secure_b = deepcopy(secure_a) - shred!(secure_a) - - @test secure_a != "foo" - @test secure_b == "foo" - end -end From cf0a72ab69ab72556c18a62643fd6a9e40232b76 Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Thu, 14 Jun 2018 14:02:50 -0400 Subject: [PATCH 07/19] Add tests for initializers and fixup initializers --- base/secretbuffer.jl | 21 +++++++++++---------- base/util.jl | 2 +- test/choosetests.jl | 2 +- test/secretbuffer.jl | 21 ++++++++++++++++++--- 4 files changed, 31 insertions(+), 15 deletions(-) diff --git a/base/secretbuffer.jl b/base/secretbuffer.jl index c3d00093ceee6..4591e1f58e9c2 100644 --- a/base/secretbuffer.jl +++ b/base/secretbuffer.jl @@ -41,7 +41,7 @@ end convert(::Type{SecretBuffer}, s::AbstractString) = SecretBuffer(String(s)) SecretBuffer(str::AbstractString) = SecretBuffer(String(str)) function SecretBuffer(str::String) - buf = codepoints(str) + buf = codeunits(str) s = SecretBuffer(sizehint=length(buf)) for c in buf write(s, c) @@ -55,28 +55,29 @@ end Initialize a new `SecretBuffer` with `data` and securely zero the original source argument. """ -SecretBuffer!(s::Cstring) = SecretBuffer!(convert(Ptr{UInt8}, s)) -function SecretBuffer!(p::Ptr{UInt8}) - len = ccall(:strlen, Cint, (Ptr{UInt8},), p) +function SecretBuffer!(d::Vector{UInt8}) + len = length(d) s = SecretBuffer(sizehint=len) for i in 1:len - write(s, unsafe_load(p, i)) + write(s, d[i]) end seek(s, 0) - unsafe_securezero!(p, len) + securezero!(d) s end -function SecretBuffer!(d::Vector{UInt8}) - len = length(d) + +unsafe_SecretBuffer!(s::Cstring) = unsafe_SecretBuffer!(convert(Ptr{UInt8}, s), ccall(:strlen, Cint, (Cstring,), s)) +function unsafe_SecretBuffer!(p::Ptr{UInt8}, len=1) s = SecretBuffer(sizehint=len) for i in 1:len - write(s, d[i]) + write(s, unsafe_load(p, i)) end seek(s, 0) - securezero!(d) + unsafe_securezero!(p, len) s end + show(io::IO, s::SecretBuffer) = print(io, "SecretBuffer(\"*******\")") hash(s::SecretBuffer, h::UInt) = hash(SecretBuffer, hash((s.data, s.size, s.ptr), h)) diff --git a/base/util.jl b/base/util.jl index 819981c27d269..6e30caea2a7ba 100644 --- a/base/util.jl +++ b/base/util.jl @@ -467,7 +467,7 @@ function getpass(prompt::AbstractString) end else function getpass(prompt::AbstractString) - SecretBuffer!(ccall(:getpass, Cstring, (Cstring,), prompt)) + unsafe_SecretBuffer!(ccall(:getpass, Cstring, (Cstring,), prompt)) end end diff --git a/test/choosetests.jl b/test/choosetests.jl index 7614d42d6fffb..2bceb7f35e626 100644 --- a/test/choosetests.jl +++ b/test/choosetests.jl @@ -53,7 +53,7 @@ function choosetests(choices = []) "enums", "cmdlineargs", "int", "checked", "bitset", "floatfuncs", "precompile", "inline", "boundscheck", "error", "ambiguous", "cartesian", "osutils", - "channels", "iostream", "specificity", "codegen", + "channels", "iostream", "secretbuffer", "specificity", "codegen", "reinterpretarray", "syntax", "logging", "missing", "asyncmap" ] diff --git a/test/secretbuffer.jl b/test/secretbuffer.jl index 9245d5766e328..089d16167804c 100644 --- a/test/secretbuffer.jl +++ b/test/secretbuffer.jl @@ -1,5 +1,5 @@ using Base: SecretBuffer, SecretBuffer!, shred!, isshredded -using Base.Test +using Test @testset "SecretBuffer" begin @testset "original unmodified" begin @@ -10,8 +10,8 @@ using Base.Test seekstart(secret) @test shred!(secret) === secret - @test read(secret, String) != "" - @test str != "foobar" + @test read(secret, String) == "" + @test str == "foobar" end @testset "finalizer" begin @@ -25,4 +25,19 @@ using Base.Test @test all(iszero, v) @test !isshredded(secret_b) end + + @testset "initializers" begin + s1 = SecretBuffer("setec astronomy") + data2 = [0x73, 0x65, 0x74, 0x65, 0x63, 0x20, 0x61, 0x73, 0x74, 0x72, 0x6f, 0x6e, 0x6f, 0x6d, 0x79] + s2 = SecretBuffer!(data2) + @test all(==(0x0), data2) + @test s1 == s2 + + ptr3 = Base.unsafe_convert(Cstring, "setec astronomy") + s3 = Base.unsafe_SecretBuffer!(ptr3) + @test Base.unsafe_string(ptr3) == "" + @test s1 == s2 == s3 + + shred!(s1); shred!(s2); shred!(s3) + end end From 4aed397324756fe8a570bc3907dd098e176bc4fa Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Thu, 14 Jun 2018 15:50:36 -0400 Subject: [PATCH 08/19] Fixup Cstring conversions --- base/secretbuffer.jl | 9 +++++---- stdlib/LibGit2/src/callbacks.jl | 5 ++--- stdlib/LibGit2/src/gitcredential.jl | 6 +++--- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/base/secretbuffer.jl b/base/secretbuffer.jl index 4591e1f58e9c2..aa12c11c2d717 100644 --- a/base/secretbuffer.jl +++ b/base/secretbuffer.jl @@ -51,7 +51,7 @@ function SecretBuffer(str::String) end """ - SecretBuffer!(data::Union{Cstring, Ptr{UInt8}, Vector{UInt8}}) + SecretBuffer!(data::Vector{UInt8}) Initialize a new `SecretBuffer` with `data` and securely zero the original source argument. """ @@ -105,10 +105,11 @@ function write(io::IO, s::SecretBuffer) return nb end +cconvert(::Type{Cstring}, s::SecretBuffer) = unsafe_convert(Cstring, s) function unsafe_convert(::Type{Cstring}, s::SecretBuffer) # Ensure that no nuls appear in the valid region - if any(!(==(0x00)), s.data[i] for i in 1:s.size) - error("`SecretBuffers` containing nul bytes cannot be converted to `Cstring`") + if any(==(0x00), s.data[i] for i in 1:s.size) + throw(ArgumentError("`SecretBuffers` containing nul bytes cannot be converted to C strings")) end # Add a hidden nul byte just past the end of the valid region p = s.ptr @@ -116,7 +117,7 @@ function unsafe_convert(::Type{Cstring}, s::SecretBuffer) write(s, '\0') s.ptr = p s.size -= 1 - return unsafe_convert(Ptr{UInt8}, s.data) + return Cstring(unsafe_convert(Ptr{Cchar}, s.data)) end seek(io::SecretBuffer, n::Integer) = (io.ptr = max(min(n+1, io.size+1), 1); io) diff --git a/stdlib/LibGit2/src/callbacks.jl b/stdlib/LibGit2/src/callbacks.jl index cbd11a826275f..63b63f60a08ba 100644 --- a/stdlib/LibGit2/src/callbacks.jl +++ b/stdlib/LibGit2/src/callbacks.jl @@ -167,9 +167,8 @@ function authenticate_ssh(libgit2credptr::Ptr{Ptr{Cvoid}}, p::CredentialPayload, if !revised return exhausted_abort() end - return ccall((:git_cred_ssh_key_new, :libgit2), Cint, - (Ptr{Ptr{Cvoid}}, Cstring, Cstring, Cstring, Ptr{UInt8}), + (Ptr{Ptr{Cvoid}}, Cstring, Cstring, Cstring, Cstring), libgit2credptr, cred.user, cred.pubkey, cred.prvkey, cred.pass) end @@ -227,7 +226,7 @@ function authenticate_userpass(libgit2credptr::Ptr{Ptr{Cvoid}}, p::CredentialPay end return ccall((:git_cred_userpass_plaintext_new, :libgit2), Cint, - (Ptr{Ptr{Cvoid}}, Cstring, Ptr{UInt8}), + (Ptr{Ptr{Cvoid}}, Cstring, Cstring), libgit2credptr, cred.user, cred.pass) end diff --git a/stdlib/LibGit2/src/gitcredential.jl b/stdlib/LibGit2/src/gitcredential.jl index 9a60e65a86b0b..ce51ea7dd15e4 100644 --- a/stdlib/LibGit2/src/gitcredential.jl +++ b/stdlib/LibGit2/src/gitcredential.jl @@ -80,12 +80,12 @@ function Base.parse(::Type{GitCredential}, url::AbstractString) end function Base.copy!(a::GitCredential, b::GitCredential) - shred!(a) + Base.shred!(a) a.protocol = b.protocol a.host = b.host a.path = b.path a.username = b.username - a.password = copy(b.password) + a.password = b.password == nothing ? nothing : copy(b.password) return a end @@ -114,7 +114,7 @@ function Base.read!(io::IO, cred::GitCredential) if key == "url" # Any components which are missing from the URL will be set to empty # https://git-scm.com/docs/git-credential#git-credential-codeurlcode - shred!(parse(GitCredential, value)) do urlcred + Base.shred!(parse(GitCredential, value)) do urlcred copy!(cred, urlcred) end else From e223778bffa070807b7316533a06b69d164dc6f4 Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Thu, 14 Jun 2018 16:45:43 -0400 Subject: [PATCH 09/19] Qualify shred! in OldPkg --- stdlib/OldPkg/src/entry.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/stdlib/OldPkg/src/entry.jl b/stdlib/OldPkg/src/entry.jl index b4a9f56025a58..01ff9aafd2b8b 100644 --- a/stdlib/OldPkg/src/entry.jl +++ b/stdlib/OldPkg/src/entry.jl @@ -420,7 +420,7 @@ function update(branch::AbstractString, upkgs::Set{String}) end end fixed = Read.fixed(avail,instd,dont_update) - shred!(LibGit2.CachedCredentials()) do creds + Base.shred!(LibGit2.CachedCredentials()) do creds stopupdate = false for (pkg,ver) in fixed ispath(pkg,".git") || continue From 94d2ebf76892794b3ce5e3a3dcc599fbb00da438 Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Wed, 20 Jun 2018 12:23:25 -0400 Subject: [PATCH 10/19] Use constant time algorithm for ==(::SecureBuffer...) I tried removing == entirely, but that proved to be too difficult. Also remove information leaks from hash. Co-authored-by: chethega --- base/secretbuffer.jl | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/base/secretbuffer.jl b/base/secretbuffer.jl index aa12c11c2d717..611f725ed3171 100644 --- a/base/secretbuffer.jl +++ b/base/secretbuffer.jl @@ -80,8 +80,21 @@ end show(io::IO, s::SecretBuffer) = print(io, "SecretBuffer(\"*******\")") -hash(s::SecretBuffer, h::UInt) = hash(SecretBuffer, hash((s.data, s.size, s.ptr), h)) -==(s1::SecretBuffer, s2::SecretBuffer) = (view(s1.data, 1:s1.size), s1.ptr) == (view(s2.data, 1:s2.size), s2.ptr) +# Unlike other IO objects, equality is computed by value for convenience +==(s1::SecretBuffer, s2::SecretBuffer) = (s1.ptr == s2.ptr) && (s1.size == s2.size) && (UInt8(0) == _bufcmp(s1.data, s2.data, min(s1.size, s2.size))) +# Also attempt a constant time buffer comparison algorithm — the length of the secret might be +# inferred by a timing attack, but not its values. +@noinline function _bufcmp(data1::Vector{UInt8}, data2::Vector{UInt8}, sz::Int) + res = UInt8(0) + for i = 1:sz + res |= xor(data1[i], data2[i]) + end + return res +end +# All SecretBuffers hash the same to avoid leaking information or breaking consistency with == +const _sb_hash = UInt === UInt32 ? 0x111c0925 : 0xb06061e370557428 +hash(s::SecretBuffer, h::UInt) = hash(_sb_hash, h) + function write(io::SecretBuffer, b::UInt8) if io.ptr > length(io.data) From dff0faabb71d522e04d6e754ccee2eb181bc5df9 Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Wed, 20 Jun 2018 13:05:33 -0400 Subject: [PATCH 11/19] Document `SecretBuffer(::AbstractString)` --- base/secretbuffer.jl | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/base/secretbuffer.jl b/base/secretbuffer.jl index 611f725ed3171..7840a1f4b144a 100644 --- a/base/secretbuffer.jl +++ b/base/secretbuffer.jl @@ -38,7 +38,17 @@ mutable struct SecretBuffer <: IO end end -convert(::Type{SecretBuffer}, s::AbstractString) = SecretBuffer(String(s)) +""" + SecretBuffer(str::AbstractString) + +A convenience constructor to initialize a `SecretBuffer` from a non-secret string. + +Strings are bad at keeping secrets because they are unable to be securely +zeroed or destroyed. Therefore, avoid using this constructor with secret data. +Instead of starting with a string, either construct the `SecretBuffer` +incrementally with `SecretBuffer()` and `write`, or use a `Vector{UInt8}` with +the `Base.SecretBuffer!(::Vector{UInt8})` constructor. +""" SecretBuffer(str::AbstractString) = SecretBuffer(String(str)) function SecretBuffer(str::String) buf = codeunits(str) @@ -49,6 +59,7 @@ function SecretBuffer(str::String) seek(s, 0) s end +convert(::Type{SecretBuffer}, s::AbstractString) = SecretBuffer(String(s)) """ SecretBuffer!(data::Vector{UInt8}) From 34c6271631b502c5b32485726794dc3e090b0bbb Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Wed, 20 Jun 2018 13:13:11 -0400 Subject: [PATCH 12/19] Add warning about storing passwords in URLs to git_url --- stdlib/LibGit2/src/utils.jl | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/stdlib/LibGit2/src/utils.jl b/stdlib/LibGit2/src/utils.jl index dd603b8e2140f..0185fc944d866 100644 --- a/stdlib/LibGit2/src/utils.jl +++ b/stdlib/LibGit2/src/utils.jl @@ -2,6 +2,10 @@ # Parse "GIT URLs" syntax (URLs and a scp-like syntax). For details see: # https://git-scm.com/docs/git-clone#_git_urls_a_id_urls_a +# Note that using a Regex like this is inherently insecure with regards to its +# handling of passwords; we are unable to deterministically and securely erase +# the passwords from memory after use. +# TODO: reimplement with a Julian parser instead of leaning on this regex const URL_REGEX = r""" ^(?:(?ssh|git|https?)://)?+ (?: @@ -107,6 +111,11 @@ provided the URL produced will use the alternative [scp-like syntax](https://git provided. Cannot be specified when using the scp-like syntax. * `path::AbstractString=""`: the path to use in the output if provided. +!!!warning + Avoid using passwords in URLs. Unlike the credential objects, Julia is not able + to securely zero or destroy the sensitive data after use and the password may + remain in memory; possibly to be exposed by an uninitialized memory. + # Examples ```jldoctest julia> LibGit2.git_url(username="git", host="github.com", path="JuliaLang/julia.git") From 3e8960fcd556a4a0b6d0378b78b21858ba4f819c Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Thu, 21 Jun 2018 11:05:06 -0400 Subject: [PATCH 13/19] Warn if not `shred!`ed before hitting finalizer Move lower in the bootstrap order to allow `@warn`. --- base/secretbuffer.jl | 5 ++++- base/sysimg.jl | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/base/secretbuffer.jl b/base/secretbuffer.jl index 7840a1f4b144a..bf3a98fc39cf4 100644 --- a/base/secretbuffer.jl +++ b/base/secretbuffer.jl @@ -163,7 +163,10 @@ function read(io::SecretBuffer, ::Type{UInt8}) return byte end -final_shred!(s::SecretBuffer) = shred!(s) +function final_shred!(s::SecretBuffer) + !isshredded(s) && @warn("a SecretBuffer was `shred!`ed by the GC; use `shred!` manually after use to minimize exposure.") + shred!(s) +end function shred!(s::SecretBuffer) securezero!(s.data) diff --git a/base/sysimg.jl b/base/sysimg.jl index 2f6305aecb454..77d452773d631 100644 --- a/base/sysimg.jl +++ b/base/sysimg.jl @@ -246,7 +246,6 @@ include("c.jl") include("io.jl") include("iostream.jl") include("iobuffer.jl") -include("secretbuffer.jl") # strings & printing include("intfuncs.jl") @@ -334,6 +333,7 @@ using .Filesystem include("process.jl") include("grisu/grisu.jl") include("methodshow.jl") +include("secretbuffer.jl") # core math functions include("floatfuncs.jl") From a5b1baad6d5b752dc5af2288ef54ceff2aa6d963 Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Thu, 21 Jun 2018 12:44:37 -0400 Subject: [PATCH 14/19] Detangle Base.prompt and Base.getpass --- base/util.jl | 44 +++++++++++++++++++-------------- stdlib/LibGit2/src/callbacks.jl | 4 +-- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/base/util.jl b/base/util.jl index 6e30caea2a7ba..edb7e9558c838 100644 --- a/base/util.jl +++ b/base/util.jl @@ -445,9 +445,21 @@ function securezero! end ccall(:memset, Ptr{T}, (Ptr{T}, Cint, Csize_t), p, 0, len*sizeof(T)) unsafe_securezero!(p::Ptr{Cvoid}, len::Integer=1) = Ptr{Cvoid}(unsafe_securezero!(Ptr{UInt8}(p), len)) +""" + Base.getpass(message::AbstractString) -> `Base.SecretBuffer` + +Display a message and wait for the user to input a secret, returning an `IO` +object containing the secret. + +Note that on Windows, the secret might be displayed as it is typed; see +`Base.winprompt` for securely retrieving username/password pairs from a secure +graphical interface. +""" +function getpass end + if Sys.iswindows() function getpass(prompt::AbstractString) - print(prompt) + print(prompt, ':') flush(stdout) s = SecretBuffer() plen = 0 @@ -467,36 +479,30 @@ function getpass(prompt::AbstractString) end else function getpass(prompt::AbstractString) - unsafe_SecretBuffer!(ccall(:getpass, Cstring, (Cstring,), prompt)) + msg = string(prompt, ':') + unsafe_SecretBuffer!(ccall(:getpass, Cstring, (Cstring,), msg)) end end """ - prompt(message; default="", password=false) -> Union{String, SecretBuffer, Nothing} + prompt(message; default="") -> Union{String, Nothing} Displays the `message` then waits for user input. Input is terminated when a newline (\\n) is encountered or EOF (^D) character is entered on a blank line. If a `default` is provided -then the user can enter just a newline character to select the `default`. Alternatively, -when the `password` keyword is `true` the characters entered by the user will not be -displayed and it will return a `SecretBuffer` instead of a `String`. +then the user can enter just a newline character to select the `default`. + +See also `Base.getpass` and `Base.winprompt` for secure entry of passwords. """ -function prompt(message::AbstractString; default::AbstractString="", password::Bool=false) - if Sys.iswindows() && password - error("Command line prompt not supported for password entry on windows. Use `Base.winprompt` instead") - end +function prompt(message::AbstractString; default::AbstractString="") msg = !isempty(default) ? "$message [$default]:" : "$message:" - if password - # `getpass` automatically chomps. We cannot tell an EOF from a '\n'. - uinput = getpass(msg) - else - print(msg) - uinput = readline(keep=true) - isempty(uinput) && return nothing # Encountered an EOF - uinput = chomp(uinput) - end + print(msg) + uinput = readline(keep=true) + isempty(uinput) && return nothing # Encountered an EOF + uinput = chomp(uinput) isempty(uinput) ? default : uinput end + # Windows authentication prompt if Sys.iswindows() struct CREDUI_INFO diff --git a/stdlib/LibGit2/src/callbacks.jl b/stdlib/LibGit2/src/callbacks.jl index 63b63f60a08ba..ed33adc70faf8 100644 --- a/stdlib/LibGit2/src/callbacks.jl +++ b/stdlib/LibGit2/src/callbacks.jl @@ -151,7 +151,7 @@ function authenticate_ssh(libgit2credptr::Ptr{Ptr{Cvoid}}, p::CredentialPayload, response === nothing && return user_abort() cred.pass = response[2] else - response = Base.prompt("Passphrase for $(cred.prvkey)", password=true) + response = Base.getpass("Passphrase for $(cred.prvkey)") response === nothing && return user_abort() cred.pass = response isempty(cred.pass) && return user_abort() # Ambiguous if EOF or newline @@ -209,7 +209,7 @@ function authenticate_userpass(libgit2credptr::Ptr{Ptr{Cvoid}}, p::CredentialPay cred.user = response url = git_url(scheme=p.scheme, host=p.host, username=cred.user) - response = Base.prompt("Password for '$url'", password=true) + response = Base.getpass("Password for '$url'") response === nothing && return user_abort() cred.pass = response isempty(cred.pass) && return user_abort() # Ambiguous if EOF or newline From 9ffda78938909a0f243eb9eae1ff6ad5444ae760 Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Thu, 21 Jun 2018 12:57:42 -0400 Subject: [PATCH 15/19] Fixup `SecretBuffer!` docstrong from review --- base/secretbuffer.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/base/secretbuffer.jl b/base/secretbuffer.jl index bf3a98fc39cf4..6af1f2627457b 100644 --- a/base/secretbuffer.jl +++ b/base/secretbuffer.jl @@ -64,7 +64,7 @@ convert(::Type{SecretBuffer}, s::AbstractString) = SecretBuffer(String(s)) """ SecretBuffer!(data::Vector{UInt8}) -Initialize a new `SecretBuffer` with `data` and securely zero the original source argument. +Initialize a new `SecretBuffer` from `data`, securely zeroing `data` afterwards. """ function SecretBuffer!(d::Vector{UInt8}) len = length(d) From 5836427fc19a00e76263be3b9d866a8dbd51447c Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Thu, 21 Jun 2018 13:34:06 -0400 Subject: [PATCH 16/19] Add a space after `:` in prompts --- base/util.jl | 6 +- stdlib/LibGit2/test/libgit2.jl | 126 ++++++++++++++++----------------- 2 files changed, 66 insertions(+), 66 deletions(-) diff --git a/base/util.jl b/base/util.jl index edb7e9558c838..33fb589b67a7a 100644 --- a/base/util.jl +++ b/base/util.jl @@ -459,7 +459,7 @@ function getpass end if Sys.iswindows() function getpass(prompt::AbstractString) - print(prompt, ':') + print(prompt, ": ") flush(stdout) s = SecretBuffer() plen = 0 @@ -479,7 +479,7 @@ function getpass(prompt::AbstractString) end else function getpass(prompt::AbstractString) - msg = string(prompt, ':') + msg = string(prompt, ": ") unsafe_SecretBuffer!(ccall(:getpass, Cstring, (Cstring,), msg)) end end @@ -494,7 +494,7 @@ then the user can enter just a newline character to select the `default`. See also `Base.getpass` and `Base.winprompt` for secure entry of passwords. """ function prompt(message::AbstractString; default::AbstractString="") - msg = !isempty(default) ? "$message [$default]:" : "$message:" + msg = !isempty(default) ? "$message [$default]: " : "$message: " print(msg) uinput = readline(keep=true) isempty(uinput) && return nothing # Encountered an EOF diff --git a/stdlib/LibGit2/test/libgit2.jl b/stdlib/LibGit2/test/libgit2.jl index 4910ba60f64d6..721f477a2b722 100644 --- a/stdlib/LibGit2/test/libgit2.jl +++ b/stdlib/LibGit2/test/libgit2.jl @@ -2071,7 +2071,7 @@ mktempdir() do dir # ENV credentials are valid but requires a passphrase withenv("SSH_KEY_PATH" => valid_p_key) do challenges = [ - "Passphrase for $valid_p_key:" => "$passphrase\n", + "Passphrase for $valid_p_key: " => "$passphrase\n", ] err, auth_attempts, p = challenge_prompt(ssh_p_ex, challenges) @test err == git_ok @@ -2082,9 +2082,9 @@ mktempdir() do dir # credentials. Since we don't control the internals of LibGit2 though they # could also just re-call the credential callback like they do for HTTP. challenges = [ - "Passphrase for $valid_p_key:" => "foo\n", - "Private key location for 'git@github.com' [$valid_p_key]:" => "\n", - "Passphrase for $valid_p_key:" => "$passphrase\n", + "Passphrase for $valid_p_key: " => "foo\n", + "Private key location for 'git@github.com' [$valid_p_key]: " => "\n", + "Passphrase for $valid_p_key: " => "$passphrase\n", ] err, auth_attempts, p = challenge_prompt(ssh_p_ex, challenges) @test err == git_ok @@ -2092,7 +2092,7 @@ mktempdir() do dir # User sends EOF in passphrase prompt which aborts the credential request challenges = [ - "Passphrase for $valid_p_key:" => "\x04", + "Passphrase for $valid_p_key: " => "\x04", ] err, auth_attempts, p = challenge_prompt(ssh_p_ex, challenges) @test err == abort_prompt @@ -2100,7 +2100,7 @@ mktempdir() do dir # User provides an empty passphrase challenges = [ - "Passphrase for $valid_p_key:" => "\n", + "Passphrase for $valid_p_key: " => "\n", ] err, auth_attempts, p = challenge_prompt(ssh_p_ex, challenges) @test err == abort_prompt @@ -2118,7 +2118,7 @@ mktempdir() do dir withenv("SSH_KEY_PATH" => valid_key) do # User provides a valid username challenges = [ - "Username for 'github.com':" => "$username\n", + "Username for 'github.com': " => "$username\n", ] err, auth_attempts, p = challenge_prompt(ssh_u_ex, challenges) @test err == git_ok @@ -2126,7 +2126,7 @@ mktempdir() do dir # User sends EOF in username prompt which aborts the credential request challenges = [ - "Username for 'github.com':" => "\x04", + "Username for 'github.com': " => "\x04", ] err, auth_attempts, p = challenge_prompt(ssh_u_ex, challenges) @test err == abort_prompt @@ -2134,8 +2134,8 @@ mktempdir() do dir # User provides an empty username challenges = [ - "Username for 'github.com':" => "\n", - "Username for 'github.com':" => "\x04", + "Username for 'github.com': " => "\n", + "Username for 'github.com': " => "\x04", ] err, auth_attempts, p = challenge_prompt(ssh_u_ex, challenges) @test err == abort_prompt @@ -2143,10 +2143,10 @@ mktempdir() do dir # User repeatedly chooses an invalid username challenges = [ - "Username for 'github.com':" => "foo\n", - "Username for 'github.com' [foo]:" => "\n", - "Private key location for 'foo@github.com' [$valid_key]:" => "\n", - "Username for 'github.com' [foo]:" => "\x04", # Need to manually abort + "Username for 'github.com': " => "foo\n", + "Username for 'github.com' [foo]: " => "\n", + "Private key location for 'foo@github.com' [$valid_key]: " => "\n", + "Username for 'github.com' [foo]: " => "\x04", # Need to manually abort ] err, auth_attempts, p = challenge_prompt(ssh_u_ex, challenges) @test err == abort_prompt @@ -2156,7 +2156,7 @@ mktempdir() do dir # instead of the C_NULL in the other missing username tests. ssh_user_empty_ex = gen_ex(valid_cred, username="") challenges = [ - "Username for 'github.com':" => "$username\n", + "Username for 'github.com': " => "$username\n", ] err, auth_attempts, p = challenge_prompt(ssh_user_empty_ex, challenges) @test err == git_ok @@ -2177,7 +2177,7 @@ mktempdir() do dir # User provides valid credentials challenges = [ - "Private key location for 'git@github.com':" => "$valid_key\n", + "Private key location for 'git@github.com': " => "$valid_key\n", ] err, auth_attempts, p = challenge_prompt(ssh_ex, challenges) @test err == git_ok @@ -2185,8 +2185,8 @@ mktempdir() do dir # User provides valid credentials that requires a passphrase challenges = [ - "Private key location for 'git@github.com':" => "$valid_p_key\n", - "Passphrase for $valid_p_key:" => "$passphrase\n", + "Private key location for 'git@github.com': " => "$valid_p_key\n", + "Passphrase for $valid_p_key: " => "$passphrase\n", ] err, auth_attempts, p = challenge_prompt(ssh_p_ex, challenges) @test err == git_ok @@ -2194,7 +2194,7 @@ mktempdir() do dir # User sends EOF in private key prompt which aborts the credential request challenges = [ - "Private key location for 'git@github.com':" => "\x04", + "Private key location for 'git@github.com': " => "\x04", ] err, auth_attempts, p = challenge_prompt(ssh_ex, challenges) @test err == abort_prompt @@ -2202,8 +2202,8 @@ mktempdir() do dir # User provides an empty private key which triggers a re-prompt challenges = [ - "Private key location for 'git@github.com':" => "\n", - "Private key location for 'git@github.com':" => "\x04", + "Private key location for 'git@github.com': " => "\n", + "Private key location for 'git@github.com': " => "\x04", ] err, auth_attempts, p = challenge_prompt(ssh_ex, challenges) @test err == abort_prompt @@ -2212,9 +2212,9 @@ mktempdir() do dir # User provides an invalid private key until prompt limit reached. # Note: the prompt should not supply an invalid default. challenges = [ - "Private key location for 'git@github.com':" => "foo\n", - "Private key location for 'git@github.com' [foo]:" => "foo\n", - "Private key location for 'git@github.com' [foo]:" => "foo\n", + "Private key location for 'git@github.com': " => "foo\n", + "Private key location for 'git@github.com' [foo]: " => "foo\n", + "Private key location for 'git@github.com' [foo]: " => "foo\n", ] err, auth_attempts, p = challenge_prompt(ssh_ex, challenges) @test err == prompt_limit @@ -2226,7 +2226,7 @@ mktempdir() do dir withenv("SSH_KEY_PATH" => invalid_key, "SSH_PUB_KEY_PATH" => invalid_key * ".pub") do challenges = [ - "Private key location for 'git@github.com' [$invalid_key]:" => "$valid_key\n", + "Private key location for 'git@github.com' [$invalid_key]: " => "$valid_key\n", ] err, auth_attempts, p = challenge_prompt(ssh_ex, challenges) @test err == git_ok @@ -2234,9 +2234,9 @@ mktempdir() do dir # User repeatedly chooses the default invalid private key until prompt limit reached challenges = [ - "Private key location for 'git@github.com' [$invalid_key]:" => "\n", - "Private key location for 'git@github.com' [$invalid_key]:" => "\n", - "Private key location for 'git@github.com' [$invalid_key]:" => "\n", + "Private key location for 'git@github.com' [$invalid_key]: " => "\n", + "Private key location for 'git@github.com' [$invalid_key]: " => "\n", + "Private key location for 'git@github.com' [$invalid_key]: " => "\n", ] err, auth_attempts, p = challenge_prompt(ssh_ex, challenges) @test err == prompt_limit @@ -2249,8 +2249,8 @@ mktempdir() do dir @test !isfile(ENV["SSH_PUB_KEY_PATH"]) challenges = [ - # "Private key location for 'git@github.com' [$valid_key]:" => "\n" - "Public key location for 'git@github.com' [$valid_key.public]:" => "$valid_key.pub\n" + # "Private key location for 'git@github.com' [$valid_key]: " => "\n" + "Public key location for 'git@github.com' [$valid_key.public]: " => "$valid_key.pub\n" ] err, auth_attempts, p = challenge_prompt(ssh_ex, challenges) @test err == git_ok @@ -2264,8 +2264,8 @@ mktempdir() do dir @test isfile(ENV["SSH_PUB_KEY_PATH"]) challenges = [ - "Private key location for 'git@github.com' [$valid_key]:" => "\n" - "Public key location for 'git@github.com' [$invalid_key.pub]:" => "$valid_key.pub\n" + "Private key location for 'git@github.com' [$valid_key]: " => "\n" + "Public key location for 'git@github.com' [$invalid_key.pub]: " => "$valid_key.pub\n" ] err, auth_attempts, p = challenge_prompt(ssh_ex, challenges) @test err == git_ok @@ -2290,8 +2290,8 @@ mktempdir() do dir # User provides a valid username and password challenges = [ - "Username for 'https://github.com':" => "$valid_username\n", - "Password for 'https://$valid_username@github.com':" => "$valid_password\n", + "Username for 'https://github.com': " => "$valid_username\n", + "Password for 'https://$valid_username@github.com': " => "$valid_password\n", ] err, auth_attempts, p = challenge_prompt(https_ex, challenges) @test err == git_ok @@ -2299,7 +2299,7 @@ mktempdir() do dir # User sends EOF in username prompt which aborts the credential request challenges = [ - "Username for 'https://github.com':" => "\x04", + "Username for 'https://github.com': " => "\x04", ] err, auth_attempts, p = challenge_prompt(https_ex, challenges) @test err == abort_prompt @@ -2307,8 +2307,8 @@ mktempdir() do dir # User sends EOF in password prompt which aborts the credential request challenges = [ - "Username for 'https://github.com':" => "foo\n", - "Password for 'https://foo@github.com':" => "\x04", + "Username for 'https://github.com': " => "foo\n", + "Password for 'https://foo@github.com': " => "\x04", ] err, auth_attempts, p = challenge_prompt(https_ex, challenges) @test err == abort_prompt @@ -2317,8 +2317,8 @@ mktempdir() do dir # User provides an empty password which aborts the credential request since we # cannot tell it apart from an EOF. challenges = [ - "Username for 'https://github.com':" => "foo\n", - "Password for 'https://foo@github.com':" => "\n", + "Username for 'https://github.com': " => "foo\n", + "Password for 'https://foo@github.com': " => "\n", ] err, auth_attempts, p = challenge_prompt(https_ex, challenges) @test err == abort_prompt @@ -2327,12 +2327,12 @@ mktempdir() do dir # User repeatedly chooses invalid username/password until the prompt limit is # reached challenges = [ - "Username for 'https://github.com':" => "foo\n", - "Password for 'https://foo@github.com':" => "bar\n", - "Username for 'https://github.com' [foo]:" => "foo\n", - "Password for 'https://foo@github.com':" => "bar\n", - "Username for 'https://github.com' [foo]:" => "foo\n", - "Password for 'https://foo@github.com':" => "bar\n", + "Username for 'https://github.com': " => "foo\n", + "Password for 'https://foo@github.com': " => "bar\n", + "Username for 'https://github.com' [foo]: " => "foo\n", + "Password for 'https://foo@github.com': " => "bar\n", + "Username for 'https://github.com' [foo]: " => "foo\n", + "Password for 'https://foo@github.com': " => "bar\n", ] err, auth_attempts, p = challenge_prompt(https_ex, challenges) @test err == prompt_limit @@ -2423,8 +2423,8 @@ mktempdir() do dir # Confirm the private key if any other prompting is required ex = gen_ex(valid_p_cred) challenges = [ - "Private key location for 'git@github.com' [$default_key]:" => "\n", - "Passphrase for $default_key:" => "$passphrase\n", + "Private key location for 'git@github.com' [$default_key]: " => "\n", + "Passphrase for $default_key: " => "$passphrase\n", ] err, auth_attempts, p = challenge_prompt(ex, challenges) @test err == git_ok @@ -2458,7 +2458,7 @@ mktempdir() do dir # Expand tilde during the private key prompt challenges = [ - "Private key location for 'git@github.com':" => "~/valid\n", + "Private key location for 'git@github.com': " => "~/valid\n", ] err, auth_attempts, p = challenge_prompt(ssh_ex, challenges) @test err == git_ok @@ -2473,8 +2473,8 @@ mktempdir() do dir # Expand tilde during the public key prompt challenges = [ - "Private key location for 'git@github.com' [$valid_key]:" => "\n", - "Public key location for 'git@github.com' [$invalid_key.pub]:" => "~/valid.pub\n", + "Private key location for 'git@github.com' [$valid_key]: " => "\n", + "Public key location for 'git@github.com' [$invalid_key.pub]: " => "~/valid.pub\n", ] err, auth_attempts, p = challenge_prompt(ssh_ex, challenges) @test err == git_ok @@ -2595,8 +2595,8 @@ mktempdir() do dir # Add a credential into the cache ex = gen_ex() challenges = [ - "Username for 'https://github.com':" => "$valid_username\n", - "Password for 'https://$valid_username@github.com':" => "$valid_password\n", + "Username for 'https://github.com': " => "$valid_username\n", + "Password for 'https://$valid_username@github.com': " => "$valid_password\n", ] err, auth_attempts, p = challenge_prompt(ex, challenges) cache = p.cache @@ -2609,8 +2609,8 @@ mktempdir() do dir # Replace a credential in the cache ex = gen_ex(cached_cred=invalid_cred) challenges = [ - "Username for 'https://github.com' [alice]:" => "$valid_username\n", - "Password for 'https://$valid_username@github.com':" => "$valid_password\n", + "Username for 'https://github.com' [alice]: " => "$valid_username\n", + "Password for 'https://$valid_username@github.com': " => "$valid_password\n", ] err, auth_attempts, p = challenge_prompt(ex, challenges) cache = p.cache @@ -2623,9 +2623,9 @@ mktempdir() do dir # Canceling a credential request should leave the cache unmodified ex = gen_ex(cached_cred=invalid_cred) challenges = [ - "Username for 'https://github.com' [alice]:" => "foo\n", - "Password for 'https://foo@github.com':" => "bar\n", - "Username for 'https://github.com' [foo]:" => "\x04", + "Username for 'https://github.com' [alice]: " => "foo\n", + "Password for 'https://foo@github.com': " => "bar\n", + "Username for 'https://github.com' [foo]: " => "\x04", ] err, auth_attempts, p = challenge_prompt(ex, challenges) cache = p.cache @@ -2674,8 +2674,8 @@ mktempdir() do dir # Username is supplied from the git configuration file challenges = [ - "Username for 'https://github.com' [$valid_username]:" => "\n", - "Password for 'https://$valid_username@github.com':" => "$valid_password\n", + "Username for 'https://github.com' [$valid_username]: " => "\n", + "Password for 'https://$valid_username@github.com': " => "$valid_password\n", ] err, auth_attempts, p = challenge_prompt(https_ex, challenges) @test err == git_ok @@ -2770,10 +2770,10 @@ mktempdir() do dir end challenges = [ - "Username for 'https://github.com':" => "$valid_username\n", - "Password for 'https://$valid_username@github.com':" => "$valid_password\n", - "Username for 'https://myhost.com':" => "$valid_username\n", - "Password for 'https://$valid_username@myhost.com':" => "$valid_password\n", + "Username for 'https://github.com': " => "$valid_username\n", + "Password for 'https://$valid_username@github.com': " => "$valid_password\n", + "Username for 'https://myhost.com': " => "$valid_username\n", + "Password for 'https://$valid_username@myhost.com': " => "$valid_password\n", ] first_result, second_result = challenge_prompt(ex, challenges) From 3c132302e812c28a015ecf974f0c15ecc6ed7a90 Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Fri, 22 Jun 2018 11:32:23 -0400 Subject: [PATCH 17/19] Have `shred!(f, x)` return what `f(x)` returns --- base/secretbuffer.jl | 1 - 1 file changed, 1 deletion(-) diff --git a/base/secretbuffer.jl b/base/secretbuffer.jl index 6af1f2627457b..755a313777199 100644 --- a/base/secretbuffer.jl +++ b/base/secretbuffer.jl @@ -183,5 +183,4 @@ function shred!(f::Function, x) finally shred!(x) end - x end From a927bbb18bff2113849e1597faf6ae50a3be1f15 Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Thu, 21 Jun 2018 14:38:59 -0400 Subject: [PATCH 18/19] Shred! creds used in Pkg --- stdlib/Pkg/src/Operations.jl | 30 ++-- stdlib/Pkg/src/Types.jl | 267 ++++++++++++++++++----------------- 2 files changed, 151 insertions(+), 146 deletions(-) diff --git a/stdlib/Pkg/src/Operations.jl b/stdlib/Pkg/src/Operations.jl index 26b26719e9629..24fab2367b3dd 100644 --- a/stdlib/Pkg/src/Operations.jl +++ b/stdlib/Pkg/src/Operations.jl @@ -430,22 +430,24 @@ function install_git( version::Union{VersionNumber,Nothing}, version_path::String )::Nothing - creds = LibGit2.CachedCredentials() - clones_dir = joinpath(depots()[1], "clones") - ispath(clones_dir) || mkpath(clones_dir) - repo_path = joinpath(clones_dir, string(uuid)) - repo = ispath(repo_path) ? LibGit2.GitRepo(repo_path) : begin - GitTools.clone(urls[1], repo_path; isbare=true, header = "[$uuid] $name from $(urls[1])", credentials=creds) - end - git_hash = LibGit2.GitHash(hash.bytes) - for url in urls - try LibGit2.with(LibGit2.GitObject, repo, git_hash) do g + repo, git_hash = Base.shred!(LibGit2.CachedCredentials()) do creds + clones_dir = joinpath(depots()[1], "clones") + ispath(clones_dir) || mkpath(clones_dir) + repo_path = joinpath(clones_dir, string(uuid)) + repo = ispath(repo_path) ? LibGit2.GitRepo(repo_path) : begin + GitTools.clone(urls[1], repo_path; isbare=true, header = "[$uuid] $name from $(urls[1])", credentials=creds) + end + git_hash = LibGit2.GitHash(hash.bytes) + for url in urls + try LibGit2.with(LibGit2.GitObject, repo, git_hash) do g + end + break # object was found, we can stop + catch err + err isa LibGit2.GitError && err.code == LibGit2.Error.ENOTFOUND || rethrow(err) end - break # object was found, we can stop - catch err - err isa LibGit2.GitError && err.code == LibGit2.Error.ENOTFOUND || rethrow(err) + GitTools.fetch(repo, url, refspecs=refspecs, credentials=creds) end - GitTools.fetch(repo, url, refspecs=refspecs, credentials=creds) + return repo, git_hash end tree = try LibGit2.GitObject(repo, git_hash) diff --git a/stdlib/Pkg/src/Types.jl b/stdlib/Pkg/src/Types.jl index 6828bf862bf5c..f6b5ec8aac26f 100644 --- a/stdlib/Pkg/src/Types.jl +++ b/stdlib/Pkg/src/Types.jl @@ -417,43 +417,110 @@ end casesensitive_isdir(dir::String) = isdir_windows_workaround(dir) && dir in readdir(joinpath(dir, "..")) function handle_repos_develop!(ctx::Context, pkgs::AbstractVector{PackageSpec}) - creds = LibGit2.CachedCredentials() - env = ctx.env - new_uuids = UUID[] - for pkg in pkgs - pkg.repo == nothing && continue - pkg.special_action = PKGSPEC_DEVELOPED - isempty(pkg.repo.url) && set_repo_for_pkg!(env, pkg) + Base.shred!(LibGit2.CachedCredentials()) do creds + env = ctx.env + new_uuids = UUID[] + for pkg in pkgs + pkg.repo == nothing && continue + pkg.special_action = PKGSPEC_DEVELOPED + isempty(pkg.repo.url) && set_repo_for_pkg!(env, pkg) + + + if isdir_windows_workaround(pkg.repo.url) + # Developing a local package, just point `pkg.path` to it + pkg.path = abspath(pkg.repo.url) + folder_already_downloaded = true + project_path = pkg.repo.url + parse_package!(ctx, pkg, project_path) + else + # We save the repo in case another environement wants to + # develop from the same repo, this avoids having to reclone it + # from scratch. + clone_path = joinpath(depots()[1], "clones") + mkpath(clone_path) + repo_path = joinpath(clone_path, string(hash(pkg.repo.url), "_full")) + repo, just_cloned = ispath(repo_path) ? (LibGit2.GitRepo(repo_path), false) : begin + r = GitTools.clone(pkg.repo.url, repo_path) + GitTools.fetch(r, pkg.repo.url; refspecs=refspecs, credentials=creds) + r, true + end + if !just_cloned + GitTools.fetch(repo, pkg.repo.url; refspecs=refspecs, credentials=creds) + end + close(repo) + # Copy the repo to a temporary place and check out the rev + project_path = mktempdir() + cp(repo_path, project_path; force=true) + repo = LibGit2.GitRepo(project_path) + rev = pkg.repo.rev + if isempty(rev) + if LibGit2.isattached(repo) + rev = LibGit2.branch(repo) + else + rev = string(LibGit2.GitHash(LibGit2.head(repo))) + end + end + gitobject, isbranch = checkout_rev!(repo, rev) + close(repo); close(gitobject) + + parse_package!(ctx, pkg, project_path) + dev_pkg_path = joinpath(Pkg.devdir(), pkg.name) + if isdir(dev_pkg_path) + if !isfile(joinpath(dev_pkg_path, "src", pkg.name * ".jl")) + cmderror("Path `$(dev_pkg_path)` exists but it does not contain `src/$(pkg.name).jl") + else + @info "Path `$(dev_pkg_path)` exists and looks like the correct package, using existing path instead of cloning" + end + else + mkpath(dev_pkg_path) + mv(project_path, dev_pkg_path; force=true) + push!(new_uuids, pkg.uuid) + end + pkg.path = dev_pkg_path + end + @assert pkg.path != nothing + end + return new_uuids + end +end - if isdir_windows_workaround(pkg.repo.url) - # Developing a local package, just point `pkg.path` to it - pkg.path = abspath(pkg.repo.url) - folder_already_downloaded = true - project_path = pkg.repo.url - parse_package!(ctx, pkg, project_path) - else - # We save the repo in case another environement wants to - # develop from the same repo, this avoids having to reclone it - # from scratch. - clone_path = joinpath(depots()[1], "clones") - mkpath(clone_path) - repo_path = joinpath(clone_path, string(hash(pkg.repo.url), "_full")) +function handle_repos_add!(ctx::Context, pkgs::AbstractVector{PackageSpec}; upgrade_or_add::Bool=true) + Base.shred!(LibGit2.CachedCredentials()) do creds + env = ctx.env + new_uuids = UUID[] + for pkg in pkgs + pkg.repo == nothing && continue + pkg.special_action = PKGSPEC_REPO_ADDED + isempty(pkg.repo.url) && set_repo_for_pkg!(env, pkg) + clones_dir = joinpath(depots()[1], "clones") + mkpath(clones_dir) + repo_path = joinpath(clones_dir, string(hash(pkg.repo.url))) repo, just_cloned = ispath(repo_path) ? (LibGit2.GitRepo(repo_path), false) : begin - r = GitTools.clone(pkg.repo.url, repo_path) + r = GitTools.clone(pkg.repo.url, repo_path, isbare=true, credentials=creds) GitTools.fetch(r, pkg.repo.url; refspecs=refspecs, credentials=creds) r, true end - if !just_cloned - GitTools.fetch(repo, pkg.repo.url; refspecs=refspecs, credentials=creds) + info = manifest_info(env, pkg.uuid) + pinned = (info != nothing && get(info, "pinned", false)) + if upgrade_or_add && !pinned && !just_cloned + rev = pkg.repo.rev + try + GitTools.fetch(repo, pkg.repo.url; refspecs=refspecs, credentials=creds) + catch e + e isa LibGit2.GitError || rethrow(e) + cmderror("failed to fetch from $(pkg.repo.url), error: $e") + end + end + if upgrade_or_add && !pinned + rev = pkg.repo.rev + else + # Not upgrading so the rev should be the current git-tree-sha + rev = info["git-tree-sha1"] + pkg.version = VersionNumber(info["version"]) end - close(repo) - # Copy the repo to a temporary place and check out the rev - project_path = mktempdir() - cp(repo_path, project_path; force=true) - repo = LibGit2.GitRepo(project_path) - rev = pkg.repo.rev + # see if we can get rev as a branch if isempty(rev) if LibGit2.isattached(repo) rev = LibGit2.branch(repo) @@ -462,110 +529,45 @@ function handle_repos_develop!(ctx::Context, pkgs::AbstractVector{PackageSpec}) end end gitobject, isbranch = checkout_rev!(repo, rev) - close(repo); close(gitobject) - - parse_package!(ctx, pkg, project_path) - dev_pkg_path = joinpath(Pkg.devdir(), pkg.name) - if isdir(dev_pkg_path) - if !isfile(joinpath(dev_pkg_path, "src", pkg.name * ".jl")) - cmderror("Path `$(dev_pkg_path)` exists but it does not contain `src/$(pkg.name).jl") - else - @info "Path `$(dev_pkg_path)` exists and looks like the correct package, using existing path instead of cloning" - end - else - mkpath(dev_pkg_path) - mv(project_path, dev_pkg_path; force=true) - push!(new_uuids, pkg.uuid) + if !isbranch + # If the user gave a shortened commit SHA, might as well update it to the full one + pkg.repo.rev = string(LibGit2.GitHash(gitobject)) end - pkg.path = dev_pkg_path - end - @assert pkg.path != nothing - end - return new_uuids -end - -function handle_repos_add!(ctx::Context, pkgs::AbstractVector{PackageSpec}; upgrade_or_add::Bool=true) - creds = LibGit2.CachedCredentials() - env = ctx.env - new_uuids = UUID[] - for pkg in pkgs - pkg.repo == nothing && continue - pkg.special_action = PKGSPEC_REPO_ADDED - isempty(pkg.repo.url) && set_repo_for_pkg!(env, pkg) - clones_dir = joinpath(depots()[1], "clones") - mkpath(clones_dir) - repo_path = joinpath(clones_dir, string(hash(pkg.repo.url))) - repo, just_cloned = ispath(repo_path) ? (LibGit2.GitRepo(repo_path), false) : begin - r = GitTools.clone(pkg.repo.url, repo_path, isbare=true, credentials=creds) - GitTools.fetch(r, pkg.repo.url; refspecs=refspecs, credentials=creds) - r, true - end - info = manifest_info(env, pkg.uuid) - pinned = (info != nothing && get(info, "pinned", false)) - if upgrade_or_add && !pinned && !just_cloned - rev = pkg.repo.rev - try - GitTools.fetch(repo, pkg.repo.url; refspecs=refspecs, credentials=creds) - catch e - e isa LibGit2.GitError || rethrow(e) - cmderror("failed to fetch from $(pkg.repo.url), error: $e") + git_tree = LibGit2.peel(LibGit2.GitTree, gitobject) + @assert git_tree isa LibGit2.GitTree + pkg.repo.git_tree_sha1 = SHA1(string(LibGit2.GitHash(git_tree))) + version_path = nothing + folder_already_downloaded = false + if has_uuid(pkg) && has_name(pkg) + version_path = Pkg.Operations.find_installed(pkg.name, pkg.uuid, pkg.repo.git_tree_sha1) + isdir(version_path) && (folder_already_downloaded = true) + info = manifest_info(env, pkg.uuid) + if info != nothing && get(info, "git-tree-sha1", "") == string(pkg.repo.git_tree_sha1) && folder_already_downloaded + # Same tree sha and this version already downloaded, nothing left to do + pkg.version = VersionNumber(info["version"]) + continue + end end - end - if upgrade_or_add && !pinned - rev = pkg.repo.rev - else - # Not upgrading so the rev should be the current git-tree-sha - rev = info["git-tree-sha1"] - pkg.version = VersionNumber(info["version"]) - end - - # see if we can get rev as a branch - if isempty(rev) - if LibGit2.isattached(repo) - rev = LibGit2.branch(repo) + if folder_already_downloaded + project_path = version_path else - rev = string(LibGit2.GitHash(LibGit2.head(repo))) + project_path = mktempdir() + opts = LibGit2.CheckoutOptions(checkout_strategy=LibGit2.Consts.CHECKOUT_FORCE, + target_directory=Base.unsafe_convert(Cstring, project_path)) + LibGit2.checkout_tree(repo, git_tree, options=opts) end - end - gitobject, isbranch = checkout_rev!(repo, rev) - if !isbranch - # If the user gave a shortened commit SHA, might as well update it to the full one - pkg.repo.rev = string(LibGit2.GitHash(gitobject)) - end - git_tree = LibGit2.peel(LibGit2.GitTree, gitobject) - @assert git_tree isa LibGit2.GitTree - pkg.repo.git_tree_sha1 = SHA1(string(LibGit2.GitHash(git_tree))) - version_path = nothing - folder_already_downloaded = false - if has_uuid(pkg) && has_name(pkg) - version_path = Pkg.Operations.find_installed(pkg.name, pkg.uuid, pkg.repo.git_tree_sha1) - isdir(version_path) && (folder_already_downloaded = true) - info = manifest_info(env, pkg.uuid) - if info != nothing && get(info, "git-tree-sha1", "") == string(pkg.repo.git_tree_sha1) && folder_already_downloaded - # Same tree sha and this version already downloaded, nothing left to do - pkg.version = VersionNumber(info["version"]) - continue + close(repo); close(git_tree); close(gitobject) + parse_package!(ctx, pkg, project_path) + if !folder_already_downloaded + version_path = Pkg.Operations.find_installed(pkg.name, pkg.uuid, pkg.repo.git_tree_sha1) + mkpath(version_path) + mv(project_path, version_path; force=true) + push!(new_uuids, pkg.uuid) end + @assert pkg.version isa VersionNumber end - if folder_already_downloaded - project_path = version_path - else - project_path = mktempdir() - opts = LibGit2.CheckoutOptions(checkout_strategy=LibGit2.Consts.CHECKOUT_FORCE, - target_directory=Base.unsafe_convert(Cstring, project_path)) - LibGit2.checkout_tree(repo, git_tree, options=opts) - end - close(repo); close(git_tree); close(gitobject) - parse_package!(ctx, pkg, project_path) - if !folder_already_downloaded - version_path = Pkg.Operations.find_installed(pkg.name, pkg.uuid, pkg.repo.git_tree_sha1) - mkpath(version_path) - mv(project_path, version_path; force=true) - push!(new_uuids, pkg.uuid) - end - @assert pkg.version isa VersionNumber + return new_uuids end - return new_uuids end function parse_package!(ctx, pkg, project_path) @@ -790,12 +792,13 @@ function registries(; clone_default=true)::Vector{String} if clone_default if !ispath(user_regs) mkpath(user_regs) - creds = LibGit2.CachedCredentials() - printpkgstyle(stdout, :Cloning, "default registries into $user_regs") - for (reg, url) in DEFAULT_REGISTRIES - path = joinpath(user_regs, reg) - repo = GitTools.clone(url, path; header = "registry $reg from $(repr(url))", credentials = creds) - close(repo) + Base.shred!(LibGit2.CachedCredentials()) do creds + printpkgstyle(stdout, :Cloning, "default registries into $user_regs") + for (reg, url) in DEFAULT_REGISTRIES + path = joinpath(user_regs, reg) + repo = GitTools.clone(url, path; header = "registry $reg from $(repr(url))", credentials = creds) + close(repo) + end end end end From 5a50d9bbacb48694edd2cfc9f1508b284a17e89e Mon Sep 17 00:00:00 2001 From: Matt Bauman Date: Thu, 21 Jun 2018 17:47:41 -0400 Subject: [PATCH 19/19] remove redundant secure --- base/util.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/base/util.jl b/base/util.jl index 33fb589b67a7a..0a74d300a395a 100644 --- a/base/util.jl +++ b/base/util.jl @@ -452,7 +452,7 @@ Display a message and wait for the user to input a secret, returning an `IO` object containing the secret. Note that on Windows, the secret might be displayed as it is typed; see -`Base.winprompt` for securely retrieving username/password pairs from a secure +`Base.winprompt` for securely retrieving username/password pairs from a graphical interface. """ function getpass end