#116 dkim_getsighdr() can return incorrect signature

v2.5.5
closed-fixed
3
2008-06-11
2008-06-04
M T
No

The DKIM signature is canonicalized and signed by calling dkim_getsighdr() with margins (dkim_getsighdr(dkim, tmp + n, sizeof tmp - n, DKIM_HDRMARGIN, strlen(DKIM_SIGNHEADER) + 2)). This means that for some messages, calling dkim_getsighdr with margin=0 and initial=0 results in an incorrect signature.

Specifically, the margin code prints a "\r\n\t " for some "h=" lines but the non-margin code does not have the extra space, causing some signatures to fail. A sample message for which this occurs is attached.

Discussion

  • M T
    M T
    2008-06-04

    Sample message causing invalid sig for margin=0, initial=0

     
    Attachments
    • labels: --> Functionality
    • milestone: --> v2.5.5
    • assigned_to: nobody --> sm-msk
     
  • Logged In: YES
    user_id=1048957
    Originator: NO

    dkim_eom_sign() calls dkim_getsighdr() with a fixed constant margin of DKIM_HDRMARGIN, which is obviously incompatible with the signed header generated with "margin" or "initial" set to zero (or possibly any other value).

    I suspect the solution is to make the margin a property of the DKIM handle or the library handle and not a parameter to dkim_getsighdr() so that symmetry is assured.

     
    • priority: 5 --> 3
     
  • Logged In: YES
    user_id=1048957
    Originator: NO

    Actually, dkim_getsighdr() documented this limitation already.

    I won't hold up 2.6.0 for this, but I do have a patch coming.

     
  • Logged In: YES
    user_id=1048957
    Originator: NO

    The attached patch removes the "margin" parameter from
    dkim_getsighdr(). The margin parameter is now an element
    of the DKIM handle and can be changed with the new
    dkim_set_margin() function. This is done to allow the
    signing code to use a user-provided margin instead of using
    the fixed value (which somewhat defeats the purpose of
    taking it as a parameter in the first place).

    File Added: PATCH

     
  • Proposed patch #1

     
    Attachments
  • Logged In: YES
    user_id=1048957
    Originator: NO

    Beta4 has now been posted which includes this patch. Also, all of the libdkim unit tests have been adjusted to use the adjusted API, and a new unit test has been added to verify zero-margin signing.

     
  • Logged In: YES
    user_id=1048957
    Originator: NO

    v2.6.0 released, containing this patch.

     
    • status: open --> closed-fixed