Update ssh folder permissions check in SSHD #761
Conversation
|
|
||
| adminSid = (PSID)malloc(SECURITY_MAX_SID_SIZE); | ||
| if (adminSid != NULL) { | ||
| if (CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, adminSid, &adminSidSize) != 0) { |
There was a problem hiding this comment.
Deeply nested code like this should probably be a separate function
| if (systemSid != NULL) { | ||
| if (CreateWellKnownSid(WinLocalSystemSid, NULL, systemSid, &systemSidSize) != 0) { | ||
| if (LookupAccountSidW(NULL, systemSid, systemName, &systemNameSize, systemDomain, &systemDomainSize, &sidType) != 0) { | ||
| logit("Suggest restricting write permissions on '%S' folder to %S\\%S and %S\\%S.", path_utf16, systemDomain, systemName, adminDomain, adminName); |
There was a problem hiding this comment.
The message would be more useful if instead of suggesting what it should be, indicate what makes it open
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| wchar_t* bad_user = NULL; | ||
| int ret = 0; | ||
| size_t log_msg_len = (DNLEN + 1 + UNLEN) * 2 + 3; // +3 for ", " and null terminator |
There was a problem hiding this comment.
My preference is to use const for the extra lengths instead of a comment as code and comments can get out of sync (someone modifies the code, but doesn't update the comment). So:
const size_t NULL_TERMINATOR_LEN = 1;
const size_t COMMA_SPACE_LEN = 2;This applies elsewhere as well.
| if (log_msg) | ||
| free(log_msg); |
There was a problem hiding this comment.
Braces make the code more clear and also avoids problems with future changes
| if (log_msg) | |
| free(log_msg); | |
| if (log_msg) { | |
| free(log_msg); | |
| } |
| return ret; | ||
| } | ||
|
|
||
| /* Helper function used by check_secure_folder_permission */ |
There was a problem hiding this comment.
Seems like it would be better for this comment to describe what this function does
| if (adminSid) | ||
| free(adminSid); | ||
| if (systemSid) | ||
| free(systemSid); |
There was a problem hiding this comment.
| if (adminSid) | |
| free(adminSid); | |
| if (systemSid) | |
| free(systemSid); | |
| if (adminSid) { | |
| free(adminSid); | |
| } | |
| if (systemSid) { | |
| free(systemSid); | |
| } |
|
|
||
| if (log_on_stderr == 0) { | ||
| /* log generic warning message in unlikely case that lookup for either well-known SID fails or user list is empty */ | ||
| logit("for '%S' folder, consider downgrading permissions for any users with unnecessary write access.", path_utf16); |
There was a problem hiding this comment.
Is logit() an upstream function or only in the Windows code? If the latter, seems better to have a wrapper function that only logs to ETW as that seems like the intent?
PR Summary
ProgData\sshfolder, notProgData\ssh\logsfolderPR Context
logsfolder, leading to undiagnosable crashes of the service after Windows Update Win32-OpenSSH#2282