Skip to content

Commit

Permalink
Improve Header.GetRawValue() logic
Browse files Browse the repository at this point in the history
Now that users can modify FormatOptions.Default, we need to be smarter
about whether or not we need to reformat header values in
Header.GetRawValue().

The old logic used to assume that cached raw values would always use
FormatOptions.International=False unless they were explicitly set, but
now that FormatOptions.Default.International can be True, we need to cache
that value and determine if it has changed since the rawValue was last
cached.

Realistically, we should be caching other FormatOptions values as well,
but this was the most important one (and the only one that was used to
determine whether the rawValue needed to be reformatted or not).

Also modified the ARC and DKIM verifier logic to share code that clones
the user-requested FormatOptions to override certain values like
NewLineFormat to also set VerifyingSignature = True.

This *may* solve issue #1000
  • Loading branch information
jstedfast committed Feb 3, 2024
1 parent d982a8a commit e3bab7b
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 14 deletions.
11 changes: 3 additions & 8 deletions MimeKit/Cryptography/ArcVerifier.cs
Original file line number Diff line number Diff line change
Expand Up @@ -396,9 +396,6 @@ async Task<bool> VerifyArcMessageSignatureAsync (FormatOptions options, MimeMess
if (!IsEnabled (signatureAlgorithm))
return false;

options = options.Clone ();
options.NewLineFormat = NewLineFormat.Dos;

// first check the body hash (if that's invalid, then the entire signature is invalid)
if (!VerifyBodyHash (options, message, signatureAlgorithm, bodyAlgorithm, maxLength, bh))
return false;
Expand Down Expand Up @@ -433,9 +430,6 @@ async Task<bool> VerifyArcSealAsync (FormatOptions options, ArcHeaderSet[] sets,
if ((key is RsaKeyParameters rsa) && rsa.Modulus.BitLength < MinimumRsaKeyLength)
return false;

options = options.Clone ();
options.NewLineFormat = NewLineFormat.Dos;

using (var stream = new DkimSignatureStream (CreateVerifyContext (algorithm, key))) {
using (var filtered = new FilteredStream (stream)) {
filtered.Add (options.CreateNewLineFilter ());
Expand Down Expand Up @@ -673,6 +667,7 @@ async Task<ArcValidationResult> VerifyAsync (FormatOptions options, MimeMessage
int newest = count - 1;

result.Seals = new ArcHeaderValidationResult[count];
options = GetVerifyOptions (options);

// validate the most recent Arc-Message-Signature
try {
Expand Down Expand Up @@ -788,7 +783,7 @@ public Task<ArcValidationResult> VerifyAsync (FormatOptions options, MimeMessage
/// </exception>
public ArcValidationResult Verify (MimeMessage message, CancellationToken cancellationToken = default)
{
return Verify (FormatOptions.Default, message, cancellationToken);
return Verify (FormatOptions.VerifySignature, message, cancellationToken);
}

/// <summary>
Expand All @@ -811,7 +806,7 @@ public ArcValidationResult Verify (MimeMessage message, CancellationToken cancel
/// </exception>
public Task<ArcValidationResult> VerifyAsync (MimeMessage message, CancellationToken cancellationToken = default)
{
return VerifyAsync (FormatOptions.Default, message, cancellationToken);
return VerifyAsync (FormatOptions.VerifySignature, message, cancellationToken);
}
}
}
7 changes: 3 additions & 4 deletions MimeKit/Cryptography/DkimVerifier.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,7 @@ async Task<bool> VerifyAsync (FormatOptions options, MimeMessage message, Header
if (!IsEnabled (signatureAlgorithm))
return false;

options = options.Clone ();
options.NewLineFormat = NewLineFormat.Dos;
options = GetVerifyOptions (options);

// first check the body hash (if that's invalid, then the entire signature is invalid)
if (!VerifyBodyHash (options, message, signatureAlgorithm, bodyAlgorithm, maxLength, bh))
Expand Down Expand Up @@ -242,7 +241,7 @@ public Task<bool> VerifyAsync (FormatOptions options, MimeMessage message, Heade
/// </exception>
public bool Verify (MimeMessage message, Header dkimSignature, CancellationToken cancellationToken = default)
{
return Verify (FormatOptions.Default, message, dkimSignature, cancellationToken);
return Verify (FormatOptions.VerifySignature, message, dkimSignature, cancellationToken);
}

/// <summary>
Expand Down Expand Up @@ -274,7 +273,7 @@ public bool Verify (MimeMessage message, Header dkimSignature, CancellationToken
/// </exception>
public Task<bool> VerifyAsync (MimeMessage message, Header dkimSignature, CancellationToken cancellationToken = default)
{
return VerifyAsync (FormatOptions.Default, message, dkimSignature, cancellationToken);
return VerifyAsync (FormatOptions.VerifySignature, message, dkimSignature, cancellationToken);
}
}
}
15 changes: 15 additions & 0 deletions MimeKit/Cryptography/DkimVerifierBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,21 @@ public bool IsEnabled (DkimSignatureAlgorithm algorithm)
return (enabledSignatureAlgorithms & (1 << (int) algorithm)) != 0;
}

/// <summary>
/// Get the formatting options for use with verifying DKIM signatures.
/// </summary>
/// <param name="format">The user-requested formatting options.</param>
/// <returns>The formatting options.</returns>
internal static FormatOptions GetVerifyOptions (FormatOptions format)
{
var options = format.Clone ();
options.NewLineFormat = NewLineFormat.Dos;
options.VerifyingSignature = true;
options.HiddenHeaders.Clear ();
options.EnsureNewLine = false;
return options;
}

static bool IsWhiteSpace (char c)
{
return c == ' ' || c == '\t';
Expand Down
15 changes: 15 additions & 0 deletions MimeKit/FormatOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,16 @@ public class FormatOptions
bool international;
int maxLineLength;

/// <summary>
/// The default formatting options for verifying signatures.
/// </summary>
/// <remarks>
/// If a custom <see cref="FormatOptions"/> is not passed to methods such as
/// <see cref="MimeKit.Cryptography.DkimVerifier.Verify(FormatOptions,MimeMessage,Header,System.Threading.CancellationToken)"/>,
/// the default options will be used.
/// </remarks>
internal static readonly FormatOptions VerifySignature;

/// <summary>
/// The default formatting options.
/// </summary>
Expand Down Expand Up @@ -284,6 +294,11 @@ public bool AlwaysQuoteParameterValues {

static FormatOptions ()
{
VerifySignature = new FormatOptions {
NewLineFormat = NewLineFormat.Dos,
VerifyingSignature = true,
EnsureNewLine = false
};
Default = new FormatOptions ();
}

Expand Down
17 changes: 15 additions & 2 deletions MimeKit/Header.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public class Header

readonly byte[] rawField;
bool explicitRawValue;
bool international;
string textValue;
byte[] rawValue;

Expand Down Expand Up @@ -294,6 +295,7 @@ public Header (string field, string value) : this (Encoding.UTF8, field, value)
/// <param name="value">The raw value of the header.</param>
protected Header (ParserOptions options, HeaderId id, string name, byte[] field, byte[] value)
{
// FIXME: This .ctor should become private and we should modify it to be exactly what the Clone() method needs.
Options = options;
rawField = field;
rawValue = value;
Expand Down Expand Up @@ -404,6 +406,7 @@ public Header Clone ()
{
return new Header (Options, Id, Field, rawField, rawValue) {
explicitRawValue = explicitRawValue,
international = international,
IsInvalid = IsInvalid,

// if the textValue has already been calculated, set it on the cloned header as well.
Expand Down Expand Up @@ -1379,9 +1382,15 @@ protected virtual byte[] FormatRawValue (FormatOptions format, Encoding encoding
}
}

bool FormattingOptionsChanged (FormatOptions format)
{
// TODO: If/when we ever start caching other FormatOptions values, we'll need to update this logic.
return format.International != international;
}

internal byte[] GetRawValue (FormatOptions format)
{
if (format.International && !explicitRawValue) {
if (!explicitRawValue && !format.VerifyingSignature && FormattingOptionsChanged (format)) {
switch (Id) {
case HeaderId.DispositionNotificationTo:
case HeaderId.ResentReplyTo:
Expand Down Expand Up @@ -1471,9 +1480,10 @@ public void SetValue (FormatOptions format, Encoding encoding, string value)
textValue = Unfold (value.Trim ());

rawValue = FormatRawValue (format, encoding, textValue);
international = format.International;
explicitRawValue = false;

// cache the formatting options that change the way the header is formatted
// TODO: cache the formatting options that change the way the header is formatted
//allowMixedHeaderCharsets = format.AllowMixedHeaderCharsets;
//newLineFormat = format.NewLineFormat;
//international = format.International;
Expand Down Expand Up @@ -1587,7 +1597,10 @@ public void SetRawValue (byte[] value)
if (value.Length == 0 || value[value.Length - 1] != (byte) '\n')
throw new ArgumentException ("The raw value MUST end with a new-line character.", nameof (value));

// Note: We set international = true even though we don't actually know if the value contains UTF-8 or not
// but it shouldn't matter because explicitRawValue will prevent reformatting the value anyway.
explicitRawValue = true;
international = true;
rawValue = value;
textValue = null;

Expand Down

0 comments on commit e3bab7b

Please sign in to comment.