Message ID | 20240102191514.2583-2-soekkle@freenet.de (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | Replace SID with domain/username on Windows | expand |
Sören Krecker <soekkle@freenet.de> writes: > Replace SID with domain/username in error message, if owner of repository > and user are not equal on windows systems. Each user should have a unique > SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers). That paragraph your URL refers to does say that a SID that is used for an account will never be reused to identify a different account. But I am not sure if it means a user will never be assigned more than one SID (in other words, the reverse is not necessarily true). The paragraph also mentions that a SID can identify a non-user entity like a computer account (as opposed to "a user account")---I do not know what its implications are in the context of this patch, though. > This means that domain/username is not a loss of information. This statement does not (grammatically) make sense, but more importantly, loss of information may not be a bad thing in this case. If more than one SIDs are given to a user account and processes working for that account, these different SIDs may be translated, by using LookupAccountSidA(), to the same string for a single user@domain, and it would be an operation that loses information in that sense. But if what we *care* about is user@domain between the current process and the owner of the directory in question being the same (or not), then such a loss of information is a *good* thing. So I dunno. Arguing what we care about (is that exact SID equality between the "owner of the directory" and the "user, which the current process is working on behalf of", or do we care about the equality of the "accounts"?) may be a better way to justify this change, if you ask me. > +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str) > +{ > + SID_NAME_USE pe_use; > + DWORD len_user = 0, len_domain = 0; > + BOOL translate_sid_to_user; > + > + /* returns only FALSE, because the string pointers are NULL*/ > + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain, > + &pe_use); > + /*Alloc needed space of the strings*/ > + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); > + translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user, > + *str, &len_domain, &pe_use); > + if (translate_sid_to_user == FALSE) { > + FREE_AND_NULL(*str); > + } Style: do not enclose a single-statement block inside {}. > + else > + (*str)[len_domain] = '/'; > + return translate_sid_to_user; > +} > @@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report) > } else if (report) { > LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; > > - if (ConvertSidToStringSidA(sid, &str1)) > + if (user_sid_to_user_name(sid, &str1)) > + to_free1 = str1; > + else if (ConvertSidToStringSidA(sid, &str1)) > to_free1 = str1; Do these two helper functions return pointers pointing into the same kind of memory that you can free with the same function? That is ... > ... > "'%s' is owned by:\n" > "\t'%s'\nbut the current user is:\n" > "\t'%s'\n", path, str1, str2); > - LocalFree(to_free1); > - LocalFree(to_free2); > + free(to_free1); > + free(to_free2); ... the original code seems to say that the piece of memory we obtain from ConvertSidToStringSidA() must not be freed by calling free() but use something special called LocalFree(). I am assuing that your user_sid_to_user_name() returns a regular piece of memory that can be freed by calling regular free()? Do we need to keep track of where we got the memory from and use different function to free each variable, or something (again I do not do Windows so I'll defer all of these to Dscho, who is CC'ed this time). Thanks and a happy new year. > } > }
On Tue, 2 Jan 2024, Junio C Hamano wrote: > Sören Krecker <soekkle@freenet.de> writes: > >> Replace SID with domain/username in error message, if owner of repository >> and user are not equal on windows systems. Each user should have a unique >> SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers). > > That paragraph your URL refers to does say that a SID that is used > for an account will never be reused to identify a different account. > But I am not sure if it means a user will never be assigned more > than one SID (in other words, the reverse is not necessarily true). To my knowledge a user account will never have multiple active SIDs, but the documentation of LookupAccountSidA [1] explicitly mentions that it does look up historic SIDs. > In addition to looking up SIDs for local accounts, local domain > accounts, and explicitly trusted domain accounts, LookupAccountSid can > look up SIDs for any account in any domain in the forest, including SIDs > that appear only in the SIDhistory field of an account in the forest. > The SIDhistory field stores former SIDs of an account that has been > moved from another domain. [1] https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-lookupaccountsida#remarks > > The paragraph also mentions that a SID can identify a non-user > entity like a computer account (as opposed to "a user account")---I > do not know what its implications are in the context of this patch, > though. > >> This means that domain/username is not a loss of information. > > This statement does not (grammatically) make sense, but more > importantly, loss of information may not be a bad thing in this > case. If more than one SIDs are given to a user account and > processes working for that account, these different SIDs may be > translated, by using LookupAccountSidA(), to the same string for a > single user@domain, and it would be an operation that loses > information in that sense. > > But if what we *care* about is user@domain between the current > process and the owner of the directory in question being the same > (or not), then such a loss of information is a *good* thing. This patch only changes the output of our error message, though. It does not change what ownership information we actually compare. So if we had a hypothetical user Bob that was part of the domain example.com (SID S-1-5-21-100000001-1000000001-10000001-1001) and had been moved over from the example.org domain (old SID S-1-5-21- 2000000002-2000000002-20000002-2002) and we would detect a repository owned by bobs old SID, we would now lookup the old SID, find it attached to a user named example.com\Bob, look up Bobs current SID, find it belongs to a user named example.com\Bob and print a confusing error message. > So I dunno. Arguing what we care about (is that exact SID equality > between the "owner of the directory" and the "user, which the > current process is working on behalf of", or do we care about the > equality of the "accounts"?) may be a better way to justify this > change, if you ask me. > >> +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str) >> +{ >> + SID_NAME_USE pe_use; >> + DWORD len_user = 0, len_domain = 0; >> + BOOL translate_sid_to_user; >> + >> + /* returns only FALSE, because the string pointers are NULL*/ >> + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain, >> + &pe_use); >> + /*Alloc needed space of the strings*/ >> + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); >> + translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user, >> + *str, &len_domain, &pe_use); >> + if (translate_sid_to_user == FALSE) { >> + FREE_AND_NULL(*str); >> + } > > Style: do not enclose a single-statement block inside {}. > >> + else >> + (*str)[len_domain] = '/'; >> + return translate_sid_to_user; >> +} > >> @@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report) >> } else if (report) { >> LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; >> >> - if (ConvertSidToStringSidA(sid, &str1)) >> + if (user_sid_to_user_name(sid, &str1)) >> + to_free1 = str1; >> + else if (ConvertSidToStringSidA(sid, &str1)) >> to_free1 = str1; > > Do these two helper functions return pointers pointing into the same > kind of memory that you can free with the same function? That is ... > >> ... >> "'%s' is owned by:\n" >> "\t'%s'\nbut the current user is:\n" >> "\t'%s'\n", path, str1, str2); >> - LocalFree(to_free1); >> - LocalFree(to_free2); >> + free(to_free1); >> + free(to_free2); > > ... the original code seems to say that the piece of memory we > obtain from ConvertSidToStringSidA() must not be freed by calling > free() but use something special called LocalFree(). I am assuing > that your user_sid_to_user_name() returns a regular piece of memory > that can be freed by calling regular free()? Do we need to keep > track of where we got the memory from and use different function to > free each variable, or something (again I do not do Windows so I'll > defer all of these to Dscho, who is CC'ed this time). > > Thanks and a happy new year. > >> } >> } > >
Matthias Aßhauer <mha1993@live.de> writes: > This patch only changes the output of our error message, though. > It does not change what ownership information we actually compare. > So if we had a hypothetical user Bob that was part of the domain > example.com (SID S-1-5-21-100000001-1000000001-10000001-1001) and > had been moved over from the example.org domain (old SID S-1-5-21- > 2000000002-2000000002-20000002-2002) and we would detect a repository > owned by bobs old SID, we would now lookup the old SID, find it > attached to a user named example.com\Bob, look up Bobs current SID, > find it belongs to a user named example.com\Bob and print a confusing > error message. Yup, that is exactly the kind of breakage I was worried about. Perhaps we should do something along the lines of ... - The erroring out should be done purely by SID comparison, as that is what we have been doing to protect the users. - When creating a message, use LookupAccountSidA() to come up with a pair of domain\user strings for the directory and the process to be used in the error message: - If they are different (which is expected to be the normal case), we just use the pair of strings. - If they are the same, show old and new SID in stringified form (hopefully different SIDs would strigify to different strings?), and optionally we give the domain\user string next to it. ... then? Then we would emit an error message (in the best case) 'directory' is owned by: 'bob@example.org' but the current user is: 'charlie@example.com' and in a bad case we would instead see something like: 'directory' is owned by: SID S-1-5-21-100000001-1000000001-10000001-1001 ('bob@example.org') but the current user is: SID S-1-5-21-200000002-2000000002-20000002-2002 ('bob@example.org') which may still be serviceable. I dunno.
diff --git a/compat/mingw.c b/compat/mingw.c index 42053c1f65..bfd9573a29 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2684,6 +2684,27 @@ static PSID get_current_user_sid(void) return result; } +static BOOL user_sid_to_user_name(PSID sid, LPSTR *str) +{ + SID_NAME_USE pe_use; + DWORD len_user = 0, len_domain = 0; + BOOL translate_sid_to_user; + + /* returns only FALSE, because the string pointers are NULL*/ + LookupAccountSidA(NULL, sid, NULL, &len_user, NULL, &len_domain, + &pe_use); + /*Alloc needed space of the strings*/ + ALLOC_ARRAY((*str), (size_t)len_domain + (size_t)len_user); + translate_sid_to_user = LookupAccountSidA(NULL, sid, (*str) + len_domain, &len_user, + *str, &len_domain, &pe_use); + if (translate_sid_to_user == FALSE) { + FREE_AND_NULL(*str); + } + else + (*str)[len_domain] = '/'; + return translate_sid_to_user; +} + static int acls_supported(const char *path) { size_t offset = offset_1st_component(path); @@ -2767,7 +2788,9 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report) } else if (report) { LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; - if (ConvertSidToStringSidA(sid, &str1)) + if (user_sid_to_user_name(sid, &str1)) + to_free1 = str1; + else if (ConvertSidToStringSidA(sid, &str1)) to_free1 = str1; else str1 = "(inconvertible)"; @@ -2776,7 +2799,10 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report) str2 = "(none)"; else if (!IsValidSid(current_user_sid)) str2 = "(invalid)"; - else if (ConvertSidToStringSidA(current_user_sid, &str2)) + else if (user_sid_to_user_name(current_user_sid, &str2)) + to_free2 = str2; + else if (ConvertSidToStringSidA(current_user_sid, + &str2)) to_free2 = str2; else str2 = "(inconvertible)"; @@ -2784,8 +2810,8 @@ int is_path_owned_by_current_sid(const char *path, struct strbuf *report) "'%s' is owned by:\n" "\t'%s'\nbut the current user is:\n" "\t'%s'\n", path, str1, str2); - LocalFree(to_free1); - LocalFree(to_free2); + free(to_free1); + free(to_free2); } }
Replace SID with domain/username in error message, if owner of repository and user are not equal on windows systems. Each user should have a unique SID (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers#what-are-security-identifiers). This means that domain/username is not a loss of information. If the translation fails the message contains the SID as string. Old Prompted error message: ''' fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git' 'C:/Users/test/source/repos/git' is owned by: 'S-1-5-21-571067702-4104414259-3379520149-500' but the current user is: 'S-1-5-21-571067702-4104414259-3379520149-1001' To add an exception for this directory, call: git config --global --add safe.directory C:/Users/test/source/repos/git ''' New prompted error massage: ''' fatal: detected dubious ownership in repository at 'C:/Users/test/source/repos/git' 'C:/Users/test/source/repos/git' is owned by: 'DESKTOP-L78JVA6/Administrator' but the current user is: 'DESKTOP-L78JVA6/test' To add an exception for this directory, call: git config --global --add safe.directory C:/Users/test/source/repos/git ''' Signed-off-by: Sören Krecker <soekkle@freenet.de> --- compat/mingw.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)