Calculating MIC for signed message when *not* having MDN options fails

[kukel]

Version: AS2-LIB 4.1.1-SNAPSHOT from 'master'

I'm sending (or at least trying to send) an encrypted and signed message while at the same time not requesting an MDN and (sort of consequently) not having an 'MDNOptions' on the partnership. From our documentation, it is a valid (albeit strange) test case.

Securing the message happens in handle() in AS2SenderModule

      // compress and/or sign and/or encrypt the message if needed
      final MimeBodyPart aSecuredData = secure (aMsg);

      // Calculate MIC after compress/sign/crypt was handled, because the
      // message data might change if compression before signing is active.
      final String sMIC = calculateAndStoreMIC (aMsg);

      if (LOGGER.isDebugEnabled ())
        LOGGER.debug ("Setting message content type to '" + aSecuredData.getContentType () + "'");
      aMsg.setContentType (aSecuredData.getContentType ());

The securing part works and in the secure(aMsg) the signing algorithm is taken from the partnership:

final String sSignAlgorithm = aPartnership.getSigningAlgorithm ();

After the signing the MIC is calculated and this part fails with the following exception

15:33:58.042 [main] ERROR com.helger.as2lib.client.AS2Client - Error sending AS2 message
com.helger.as2lib.exception.WrappedOpenAS2Exception: java.lang.NullPointerException: The value of 'DigestAlgorithm' may not be null!
	at com.helger.as2lib.exception.WrappedOpenAS2Exception.wrap(WrappedOpenAS2Exception.java:66) ~[classes/:na]
	at com.helger.as2lib.processor.sender.AS2SenderModule.handle(AS2SenderModule.java:810) ~[classes/:na]
	at com.helger.as2lib.client.AS2Client.sendSynchronous(AS2Client.java:388) ~[classes/:na]
	at com.descartes.as2.AS2ClientBuilder.sendSynchronous(AS2ClientBuilder.java:954) [classes/:na]
	at com.descartes.as2.MainAS2TestClient.main(MainAS2TestClient.java:193) [test-classes/:na]
Caused by: java.lang.NullPointerException: The value of 'DigestAlgorithm' may not be null!
	at com.helger.commons.ValueEnforcer.notNull(ValueEnforcer.java:277) ~[ph-commons-9.1.3.jar:9.1.3]
	at com.helger.as2lib.crypto.BCCryptoHelper.calculateMIC(BCCryptoHelper.java:252) ~[classes/:na]
	at com.helger.as2lib.processor.sender.AS2SenderModule.calculateAndStoreMIC(AS2SenderModule.java:257) ~[classes/:na]
	at com.helger.as2lib.processor.sender.AS2SenderModule.handle(AS2SenderModule.java:780) ~[classes/:na]
	... 3 common frames omitted

The reason being that in protected String calculateAndStoreMIC (@Nonnull final AS2Message aMsg) Disposition options (a string here!) are being parsed to a DispositionOptions object and from that the fist mic is read.

final String sDispositionOptions = aPartnership.getAS2MDNOptions ();
final DispositionOptions aDispositionOptions = DispositionOptions.createFromString (sDispositionOptions);
. . .
final String sMIC = AS2Helper.getCryptoHelper ().calculateMIC (aMsg.getData (),
                                                               aDispositionOptions.getFirstMICAlg (),
                                                               bIncludeHeadersInMIC); (sDispositionOptions);

It looks like this is a copy/paste from an MDN related method and since it is only used in the AS2SenderModule, I think

aDispositionOptions.getFirstMICAlg (),

should be replaced by

ECryptoAlgorithmSign.getFromIDOrThrow(aPartnership.getSigningAlgorithm()),

Or an assignment, a null check like for the encryption or...

[kukel]

This is what I ended up with:

final ECryptoAlgorithmSign eSigningAlgorithm = ECryptoAlgorithmSign.getFromIDOrNull(aPartnership.getSigningAlgorithm());

if (eSigningAlgorithm == null)
    throw new OpenAS2Exception ("The signing algorithm '" + aPartnership.getSigningAlgorithm() + "' is not supported!");
final String sMIC = AS2Helper.getCryptoHelper ().calculateMIC (aMsg.getData (),eSigningAlgorithm,

And the following piece of code is superfluous then in the calculateMic method:

final String sDispositionOptions = aPartnership.getAS2MDNOptions ();
final DispositionOptions aDispositionOptions = DispositionOptions.createFromString (sDispositionOptions);

if (LOGGER.isDebugEnabled ())
  LOGGER.debug ("DispositionOptions=" + aDispositionOptions);

Will setup my github correctly so I can create PR's

[phax]

Thx. Makes sense. Btw which algo is it?
Sorry, misread it

[kukel]

I do not completely understand your question in this context. One of the reasons we are looking for newer software is that ours does not support modern algorithms and sha-1 is the max.

(Sorry, closed by accident)

[phax]

Well, this is a tricky one. There are the Disposition-Notification-Options and there is the partnership signing algorithm.

What you are outlining suits my understanding:

• AS2SenderModule must use the partnership signing algorithm
• MDN creation must use the Disposition-Notification-Options

[kukel]

Yes this is what I think it should be... But in the MDN creation it still works, unrelated pieces of code. So the fix I proposed is safe (afaics)

[phax]

I changed it slightly, so that a default algorithm is used (depending on the algorithm suite used), if none, or an invalid one is provided.

[phax]

SNAPSHOT 12 and onwards contain this fix. Thanks again for pointing it out.

[kukel]

Providing a default if none is provided sounds ok, but I am not a fan of defaults if invalid ones are provided. It's masking wrong configurations.

[phax]

That's why I added an extra logging, so that it gets obvious

[kukel]

Thankls, that makes it better (although logging tends to be 'neglected' way too often is our experience if there is additional (error) logging that should be ignored)