From 550c0ba3a9378a6759c4918c12cc3be9cc6f6a6d Mon Sep 17 00:00:00 2001 From: Dmitry Yakimenko Date: Thu, 14 Nov 2024 17:05:15 +0100 Subject: [PATCH] LastPass: handle the null logger in LP/Duo code (fixes #142) --- src/Common/ISecureLogger.cs | 5 +- src/LastPass/Client.cs | 9 +- test/LastPass/ClientTest.cs | 178 +++++++++++++++++++++++++++++++++++- test/RestFlow.cs | 18 +++- 4 files changed, 199 insertions(+), 11 deletions(-) diff --git a/src/Common/ISecureLogger.cs b/src/Common/ISecureLogger.cs index 3475bfbf..652802df 100644 --- a/src/Common/ISecureLogger.cs +++ b/src/Common/ISecureLogger.cs @@ -38,13 +38,12 @@ public void Log(LogEntry entry) { } internal class TaggedLogger(string tag, ISecureLogger logger) : ICensoredLogger { public string Tag { get; } = tag; + public ISecureLogger Logger { get; } = logger; public List Entries { get; } = []; private readonly HashSet _textFilters = []; private readonly HashSet _regexFilters = []; - public void Clear() => Entries.Clear(); - // // ISecureLogger implementation // @@ -53,7 +52,7 @@ public void Log(LogEntry entry) { var censored = entry with { Message = Censor(entry.Message) }; Entries.Add(censored); - logger.Log(censored); + Logger.Log(censored); } // diff --git a/src/LastPass/Client.cs b/src/LastPass/Client.cs index ccb4ca64..e7e25183 100644 --- a/src/LastPass/Client.cs +++ b/src/LastPass/Client.cs @@ -391,7 +391,10 @@ ISimpleLogger logger ) { // 1. Do a normal Duo V1 first - var result = DuoV1.Authenticate(host, signature, ui, rest.Transport, new TaggedLogger($"LastPass.DuoV1", logger)); + // Allow the logger to be null for optimization purposes (saved a bunch of work in the RestClient code) + // Allow the logger to be null for optimization purposes (saved a bunch of work in the RestClient code) + var duoLogger = logger == null ? null : new TaggedLogger("LastPass.DuoV1", logger); + var result = DuoV1.Authenticate(host, signature, ui, rest.Transport, duoLogger); if (result == null) return new OobWithExtras(OobResult.Cancel); @@ -412,7 +415,9 @@ ISimpleLogger logger ) { // 1. Do a normal Duo V4 first - var result = DuoV4.Authenticate(url, ui, rest.Transport, new TaggedLogger($"LastPass.DuoV4", logger)); + // Allow the logger to be null for optimization purposes (saved a bunch of work in the RestClient code) + var duoLogger = logger == null ? null : new TaggedLogger("LastPass.DuoV4", logger); + var result = DuoV4.Authenticate(url, ui, rest.Transport, duoLogger); if (result == null) return new OobWithExtras(OobResult.Cancel); diff --git a/test/LastPass/ClientTest.cs b/test/LastPass/ClientTest.cs index c1569a62..10063b0e 100644 --- a/test/LastPass/ClientTest.cs +++ b/test/LastPass/ClientTest.cs @@ -79,7 +79,7 @@ public void OpenVault_returns_accounts_with_otp() } [Fact] - public void OpenVault_returns_accounts_with_otp_and_rememeber_me() + public void OpenVault_returns_accounts_with_otp_and_remember_me() { var flow = new RestFlow() .Post(IterationResponse) @@ -130,7 +130,7 @@ public void OpenVault_returns_accounts_with_oob() } [Fact] - public void OpenVault_returns_accounts_with_oob_and_rememeber_me() + public void OpenVault_returns_accounts_with_oob_and_remember_me() { var flow = new RestFlow() .Post(IterationResponse) @@ -158,6 +158,26 @@ public void OpenVault_returns_accounts_with_oob_and_rememeber_me() Assert.NotEmpty(accounts); } + [Fact] + public void OpenVault_returns_accounts_with_duo_v4() + { + var flow = new RestFlow() + .Post(IterationResponse) + .ExpectUrl("/login.php") + .Post(DuoV4RequiredResponse) + .ExpectUrl("/login.php") + .LastPassDuo() + .Post(OkResponseValidPrivateKey) + .Get(BlobBase64) + .ExpectUrl("/getaccts.php?") + .Post("") + .ExpectUrl("/logout.php"); + + var accounts = Client.OpenVault(Username, Password, ClientInfo, GetWaitingForOobUi(), flow, ParserOptions.Default, null); + + Assert.NotEmpty(accounts); + } + [Fact] public void OpenVault_lower_cases_email() { @@ -293,6 +313,52 @@ public void OpenVault_does_not_log_to_secure_logger_when_logging_is_disabled() logger.Entries.Should().BeEmpty(); } + [Fact] + public void OpenVault_with_duo_v4_does_not_log_to_secure_logger_when_logging_is_disabled() + { + // Arrange + var flow = new RestFlow() + .Post(IterationResponse) + .ExpectUrl("/login.php") + .Post(DuoV4RequiredResponse) + .ExpectUrl("/login.php") + .LastPassDuo() + .Post(OkResponseValidPrivateKey) + .Get(BlobBase64) + .ExpectUrl("/getaccts.php?") + .Post("") + .ExpectUrl("/logout.php"); + var logger = new FakeSecureLogger(); + + // Act + Client.OpenVault(Username, Password, ClientInfo, GetWaitingForOobUi(), flow, ParserOptions.Default, logger); + + // Assert + logger.Entries.Should().BeEmpty(); + } + + [Fact] + public void OpenVault_with_duo_v4_does_not_log_to_secure_logger_when_logger_is_null() + { + // Arrange + var flow = new RestFlow() + .Post(IterationResponse) + .ExpectUrl("/login.php") + .Post(DuoV4RequiredResponse) + .ExpectUrl("/login.php") + .LastPassDuo() + .Post(OkResponseValidPrivateKey) + .Get(BlobBase64) + .ExpectUrl("/getaccts.php?") + .Post("") + .ExpectUrl("/logout.php"); + + // Act + Client.OpenVault(Username, Password, ClientInfo, GetWaitingForOobUi(), flow, ParserOptions.Default, null); + + // Nothing to assert. We expect no exceptions. + } + [Fact] public void OpenVault_attaches_log_to_exception() { @@ -1114,6 +1180,18 @@ private static void AssertSessionCommon(Session session) private const string OobRequiredResponse = "" + "" + ""; + private const string DuoV4RequiredResponse = """ + + + + """; + private const string OobRetryResponse = "" + "" + ""; private static readonly string IterationResponse = "" + $"" + ""; @@ -1121,4 +1199,100 @@ private static void AssertSessionCommon(Session session) private const string ServerResponse = "" + "" + ""; } + + internal static class RestFlowDuoExtensions + { + // TODO: Move this part to DuoV4Test + public static RestFlow Duo(this RestFlow flow) + { + return flow.Get(DuoMainHtmlResponse) // Duo: main frame + .Post("") // Duo: submit system parameters + .Get(DuoDevicesResponse) // Duo: get devices + .Post(DuoSubmitFactorResponse) // Duo: + .Post(DuoStatusSuccessResponse) + .Post("", responseUrl: "https://duo.done?code=duo_code&state=duo_state"); + } + + public static RestFlow LastPassDuo(this RestFlow flow) + { + return flow.Duo().Post(LmiapiDuoResponse); + } + + private const string DuoMainHtmlResponse = """ + + +
+ +
+ + + """; + + private const string DuoDevicesResponse = """ + { + "stat": "OK", + "response": { + "phones": [ + { + "key": "PHONE1", + "name": "Phone 1", + "sms_batch_size": 10, + "next_passcode": "1", + "index": "phone1", + "requires_compliance_text": true, + "keypress_confirm": "", + "end_of_number": "1111", + "mobile_otpable": true + }, + { + "key": "PHONE2", + "name": "Phone 2", + "sms_batch_size": 10, + "next_passcode": "1", + "index": "phone2", + "requires_compliance_text": true, + "keypress_confirm": "", + "end_of_number": "2222", + "mobile_otpable": true + } + ], + "auth_method_order": [ + { "deviceKey": "PHONE1", "factor": "Duo Push" }, + { "deviceKey": "PHONE2", "factor": "Duo Push" }, + { "deviceKey": "PHONE1", "factor": "SMS Passcode" }, + { "deviceKey": "PHONE2", "factor": "SMS Passcode" }, + { "deviceKey": "PHONE1", "factor": "Phone Call" }, + { "deviceKey": "PHONE2", "factor": "Phone Call" } + ] + } + } + """; + + private const string DuoSubmitFactorResponse = """ + { + "stat": "OK", + "response": { + "txid": "duo_txid" + } + } + """; + + private const string DuoStatusSuccessResponse = """ + { + "stat": "OK", + "response": { + "status_code": "allow", + "result": "SUCCESS", + "reason": "User approved", + } + } + """; + + private const string LmiapiDuoResponse = """ + { + "status": "allowed", + "oneTimeToken": "duo_one_time_token" + } + """; + } } diff --git a/test/RestFlow.cs b/test/RestFlow.cs index 600aa59f..7570f35a 100644 --- a/test/RestFlow.cs +++ b/test/RestFlow.cs @@ -49,6 +49,7 @@ public RestFlow Get( HttpStatusCode status = HttpStatusCode.OK, Dictionary headers = null, Dictionary cookies = null, + string responseUrl = null, Exception error = null ) { @@ -59,6 +60,7 @@ public RestFlow Get( status: status, headers: headers ?? NoHeaders, cookies: cookies ?? NoCookies, + responseUrl: responseUrl, error: error ) ); @@ -67,7 +69,7 @@ public RestFlow Get( public RestFlow Get(ResponseContent response, Exception error) { - return Get(response, HttpStatusCode.OK, NoHeaders, NoCookies, error); + return Get(response, HttpStatusCode.OK, NoHeaders, NoCookies, NoResponseUrl, error); } public RestFlow Get(Exception error) @@ -84,6 +86,7 @@ public RestFlow Post( HttpStatusCode status = HttpStatusCode.OK, Dictionary headers = null, Dictionary cookies = null, + string responseUrl = null, Exception error = null ) { @@ -94,6 +97,7 @@ public RestFlow Post( status: status, headers: headers ?? NoHeaders, cookies: cookies ?? NoCookies, + responseUrl: responseUrl, error: error ) ); @@ -102,7 +106,7 @@ public RestFlow Post( public RestFlow Post(ResponseContent response, Exception error) { - return Post(response, HttpStatusCode.OK, NoHeaders, NoCookies, error); + return Post(response, HttpStatusCode.OK, NoHeaders, NoCookies, NoResponseUrl, error); } public RestFlow Post(Exception error) @@ -119,6 +123,7 @@ public RestFlow Put( HttpStatusCode status = HttpStatusCode.OK, Dictionary headers = null, Dictionary cookies = null, + string responseUrl = null, Exception error = null ) { @@ -129,6 +134,7 @@ public RestFlow Put( status: status, headers: headers ?? NoHeaders, cookies: cookies ?? NoCookies, + responseUrl: responseUrl, error: error ) ); @@ -137,7 +143,7 @@ public RestFlow Put( public RestFlow Put(ResponseContent response, Exception error) { - return Put(response, HttpStatusCode.OK, NoHeaders, NoCookies, error); + return Put(response, HttpStatusCode.OK, NoHeaders, NoCookies, NoResponseUrl, error); } public RestFlow Put(Exception error) @@ -259,6 +265,7 @@ private class Response public readonly HttpStatusCode Status; public readonly Dictionary Headers; public readonly Dictionary Cookies; + public readonly string ResponseUrl; public readonly Exception Error; // Expected to be received from the caller @@ -270,6 +277,7 @@ public Response( HttpStatusCode status, Dictionary headers, Dictionary cookies, + string responseUrl, Exception error ) { @@ -277,6 +285,7 @@ Exception error Status = status; Headers = headers; Cookies = cookies; + ResponseUrl = responseUrl; Error = error; Expected = new Expected(method); } @@ -407,7 +416,7 @@ RestResponse allocatedResult allocatedResult.Headers = r.Headers; allocatedResult.Error = r.Error; allocatedResult.Cookies = r.Cookies; - allocatedResult.RequestUri = uri; + allocatedResult.RequestUri = r.ResponseUrl == null ? uri : new Uri(r.ResponseUrl); switch (allocatedResult) { @@ -431,6 +440,7 @@ RestResponse allocatedResult private static readonly string[] NoFragments = new string[0]; private static readonly Dictionary NoHeaders = new Dictionary(); private static readonly Dictionary NoCookies = new Dictionary(); + private const string NoResponseUrl = null; private int _currentIndex = 0; private readonly List _responses = new List();