Skip to content

Commit

Permalink
LastPass: handle the null logger in LP/Duo code (fixes #142)
Browse files Browse the repository at this point in the history
  • Loading branch information
@detunized
detunized committed Nov 14, 2024
1 parent 58b1e82 commit 550c0ba
Show file tree
Hide file tree
Showing 4 changed files with 199 additions and 11 deletions.
5 changes: 2 additions & 3 deletions src/Common/ISecureLogger.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<LogEntry> Entries { get; } = [];

private readonly HashSet<string> _textFilters = [];
private readonly HashSet<Regex> _regexFilters = [];

public void Clear() => Entries.Clear();

//
// ISecureLogger implementation
//
Expand All @@ -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);
}

//
Expand Down
9 changes: 7 additions & 2 deletions src/LastPass/Client.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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);

Expand Down
178 changes: 176 additions & 2 deletions test/LastPass/ClientTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
{
Expand Down Expand Up @@ -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()
{
Expand Down Expand Up @@ -1114,11 +1180,119 @@ private static void AssertSessionCommon(Session session)

private const string OobRequiredResponse = "<response>" + "<error cause='outofbandrequired' outofbandtype='lastpassauth' />" + "</response>";

private const string DuoV4RequiredResponse = """
<response>
<error cause="outofbandrequired"
outofbandtype="duo"
preferduowebsdk="1"
duo_authentication_url="https://duo.url?sid=duo_sid"
duo_session_token="session_token"
duo_private_token="private_token"
/>
</response>
""";

private const string OobRetryResponse = "<response>" + "<error cause='outofbandrequired' retryid='retry-id' />" + "</response>";

private static readonly string IterationResponse = "<response>" + $"<error iterations='{IterationCount}' />" + "</response>";

private const string ServerResponse =
"<response>" + "<error server='lastpass.eu' message='our princess is in another castle' />" + "</response>";
}

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 = """
<html>
<body>
<form id="plugin_form">
<input name="_xsrf" value="duo_xsrf" />
</form>
</body>
</html>
""";

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"
}
""";
}
}
18 changes: 14 additions & 4 deletions test/RestFlow.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public RestFlow Get(
HttpStatusCode status = HttpStatusCode.OK,
Dictionary<string, string> headers = null,
Dictionary<string, string> cookies = null,
string responseUrl = null,
Exception error = null
)
{
Expand All @@ -59,6 +60,7 @@ public RestFlow Get(
status: status,
headers: headers ?? NoHeaders,
cookies: cookies ?? NoCookies,
responseUrl: responseUrl,
error: error
)
);
Expand All @@ -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)
Expand All @@ -84,6 +86,7 @@ public RestFlow Post(
HttpStatusCode status = HttpStatusCode.OK,
Dictionary<string, string> headers = null,
Dictionary<string, string> cookies = null,
string responseUrl = null,
Exception error = null
)
{
Expand All @@ -94,6 +97,7 @@ public RestFlow Post(
status: status,
headers: headers ?? NoHeaders,
cookies: cookies ?? NoCookies,
responseUrl: responseUrl,
error: error
)
);
Expand All @@ -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)
Expand All @@ -119,6 +123,7 @@ public RestFlow Put(
HttpStatusCode status = HttpStatusCode.OK,
Dictionary<string, string> headers = null,
Dictionary<string, string> cookies = null,
string responseUrl = null,
Exception error = null
)
{
Expand All @@ -129,6 +134,7 @@ public RestFlow Put(
status: status,
headers: headers ?? NoHeaders,
cookies: cookies ?? NoCookies,
responseUrl: responseUrl,
error: error
)
);
Expand All @@ -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)
Expand Down Expand Up @@ -259,6 +265,7 @@ private class Response
public readonly HttpStatusCode Status;
public readonly Dictionary<string, string> Headers;
public readonly Dictionary<string, string> Cookies;
public readonly string ResponseUrl;
public readonly Exception Error;

// Expected to be received from the caller
Expand All @@ -270,13 +277,15 @@ public Response(
HttpStatusCode status,
Dictionary<string, string> headers,
Dictionary<string, string> cookies,
string responseUrl,
Exception error
)
{
Content = content;
Status = status;
Headers = headers;
Cookies = cookies;
ResponseUrl = responseUrl;
Error = error;
Expected = new Expected(method);
}
Expand Down Expand Up @@ -407,7 +416,7 @@ RestResponse<TContent> 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)
{
Expand All @@ -431,6 +440,7 @@ RestResponse<TContent> allocatedResult
private static readonly string[] NoFragments = new string[0];
private static readonly Dictionary<string, string> NoHeaders = new Dictionary<string, string>();
private static readonly Dictionary<string, string> NoCookies = new Dictionary<string, string>();
private const string NoResponseUrl = null;

private int _currentIndex = 0;
private readonly List<Response> _responses = new List<Response>();
Expand Down

0 comments on commit 550c0ba

Please sign in to comment.