From 7baad0a4740d9c06cbd703c21e7817b23e564dd1 Mon Sep 17 00:00:00 2001
From: Tess Gauthier <tessgauthier@microsoft.com>
Date: Tue, 7 Jan 2025 10:02:04 -0500
Subject: [PATCH] make env vars optional for default allow list path (#757)

* make env vars optional for default allow list path

* add pkcs11 pester test

* use lowercasing within method
---
 contrib/win32/win32compat/ssh-agent/agent.c   | 45 ++++++++++++-------
 .../win32compat/ssh-agent/keyagent-request.c  |  7 ++-
 regress/pesterTests/KeyUtils.Tests.ps1        | 28 ++++++++++++
 3 files changed, 63 insertions(+), 17 deletions(-)

diff --git a/contrib/win32/win32compat/ssh-agent/agent.c b/contrib/win32/win32compat/ssh-agent/agent.c
index 9f3400004..96e554a58 100644
--- a/contrib/win32/win32compat/ssh-agent/agent.c
+++ b/contrib/win32/win32compat/ssh-agent/agent.c
@@ -414,26 +414,41 @@ void
 agent_initialize_allow_list() {
 	/* 
 	 * allowed paths for PKCS11 libraries,
-	 * initialize to ProgramFiles and ProgramFiles(x86) by default 
+	 * attempt to initialize to ProgramFiles and ProgramFiles(x86) by default 
 	 * upstream uses /usr/lib/* and /usr/local/lib/* 
 	 */
-	size_t prog_files_len = 0, prog_files_x86_len = 0;
-	char* prog_files = NULL, * prog_files_x86 = NULL;
+	size_t allowed_len = 0, prog_files_len = 0, prog_files_x86_len = 0;
+	char* allowed_path = NULL, *prog_files = NULL, *prog_files_x86 = NULL;
 
 	_dupenv_s(&prog_files, &prog_files_len, "ProgramFiles");
-	if (!prog_files)
-		fatal("couldn't find ProgramFiles environment variable");
-	convertToForwardslash(prog_files);
-
 	_dupenv_s(&prog_files_x86, &prog_files_x86_len, "ProgramFiles(x86)");
-	if (!prog_files_x86)
-		fatal("couldn't find ProgramFiles environment variable");
-	convertToForwardslash(prog_files_x86);
 
-	size_t allowed_providers_len = 1 + prog_files_len + 4 + prog_files_x86_len + 3;
-	allowed_providers = xmalloc(allowed_providers_len);
-	sprintf_s(allowed_providers, allowed_providers_len, "/%s/*,/%s/*", prog_files, prog_files_x86);
+	if (!prog_files && !prog_files_x86) {
+		allowed_providers = xstrdup("");
+		return;
+	}
 
-	free(prog_files);
-	free(prog_files_x86);
+	if (prog_files && prog_files_x86) {
+		allowed_len = prog_files_len + 3 + prog_files_x86_len + 1;
+		allowed_path = xmalloc(allowed_len);
+		sprintf_s(allowed_path, allowed_len, "%s\\*,%s", prog_files, prog_files_x86);
+		free(prog_files);
+		free(prog_files_x86);
+	}
+	else if (prog_files) {
+		allowed_len = prog_files_len;
+		allowed_path = prog_files;
+	}
+	else if (prog_files_x86) {
+		allowed_len = prog_files_x86_len;
+		allowed_path = prog_files_x86;
+	}
+
+	allowed_len += 3; /* for additional characters below */
+	allowed_providers = xmalloc(allowed_len);
+	sprintf_s(allowed_providers, allowed_len, "%s\\*", allowed_path);
+
+	if (allowed_path) {
+		free(allowed_path);
+	}
 }
diff --git a/contrib/win32/win32compat/ssh-agent/keyagent-request.c b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
index add0bc380..0f7d08486 100644
--- a/contrib/win32/win32compat/ssh-agent/keyagent-request.c
+++ b/contrib/win32/win32compat/ssh-agent/keyagent-request.c
@@ -677,9 +677,12 @@ int process_add_smartcard_key(struct sshbuf* request, struct sshbuf* response, s
 		goto done;
 	}
 
-	if (match_pattern_list(canonical_provider, allowed_providers, 0) != 1) {
+	to_lower_case(provider);
+	verbose("provider realpath: \"%.100s\"", provider);
+	verbose("allowed provider paths: \"%.100s\"", allowed_providers);
+	if (match_pattern_list(provider, allowed_providers, 1) != 1) {
 		verbose("refusing PKCS#11 add of \"%.100s\": "
-			"provider not allowed", canonical_provider);
+			"provider not allowed", provider);
 		goto done;
 	}
 
diff --git a/regress/pesterTests/KeyUtils.Tests.ps1 b/regress/pesterTests/KeyUtils.Tests.ps1
index ea2a47460..5881f509d 100644
--- a/regress/pesterTests/KeyUtils.Tests.ps1
+++ b/regress/pesterTests/KeyUtils.Tests.ps1
@@ -17,6 +17,7 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
             $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue
         }
 
+        $pkcs11Pin = "testpin"
         $keypassphrase = "testpassword"
         $NoLibreSSL = $OpenSSHTestInfo["NoLibreSSL"]
         if($NoLibreSSL)
@@ -298,6 +299,33 @@ Describe "E2E scenarios for ssh key management" -Tags "CI" {
             $allkeys = @(ssh-add -L)
             ValidateRegistryACL -count $allkeys.count
         }
+
+        It "$tC.$tI - ssh-add - pkcs11 library (if available)" {
+            $pkcs11Path = "C:\\Program Files\\OpenSC Project\\OpenSC\\pkcs11\\opensc-pkcs11.dll"
+            if (Test-Path $pkcs11Path) {
+                #set up SSH_ASKPASS
+                Add-PasswordSetting -Pass $pkcs11Pin
+
+                ssh-add -s "$pkcs11Path"
+                $LASTEXITCODE | Should Be 0
+                #remove SSH_ASKPASS
+                Remove-PasswordSetting
+
+                #ensure added keys are listed
+                $allkeys = ssh-add -L
+                $allKeys -notmatch "The agent has no identities." | Should Be $True
+
+                #delete added keys
+                iex "cmd /c `"ssh-add -D 2> nul `""
+
+                #check keys are deleted
+                $allkeys = ssh-add -L
+                $allKeys -match "The agent has no identities." | Should Be $True
+            }
+            else {
+                Write-Host "skipping pkcs11 test because provider not found"
+            }
+        }
     }
 
     Context "$tC ssh-keygen known_hosts operations" {