738454 - Introduce a separate error code SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert (:KaiE:)]

Attachment: Patch v1 (from Wan-Teh)

Wan-Teh proposes to introduce a separate error code for bad-signature-because-algorithm-disabled-by-policy

I'm moving Wan-Teh's patch from bug 590364 comment 25 to this bug.

Comment 1

[Kai Engert (:KaiE:)]

When using the patch from comment 25, Firefox blocks the connection and shows the following message:

  Secure Connection Failed
  An error occurred during a connection to kuix.de:9450.


No error code is shown.
This doesn't surprise me, because PSM doesn't know the name of this new error number.

I will file a PSM bug to get the error code string added to PSM.

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 608466 [details] [diff] [review]
Patch v1 (from Wan-Teh)

Wan-Teh, using your patch to NSS, and using the patch for PSM in bug 738457, we get the expected display:

  Secure Connection Failed
  An error occurred during a connection to kuix.de:9450.
  Algorithm disabled by policy.
  (Error code: sec_error_algorithm_disabled_by_policy)

That means your patch seems to work.

Who should review your patch?

Comment 3

[Kai Engert (:KaiE:)]

FYI, comment 2 

- referred to the classic verification code path
- the error is NOT overridable


In addition I performed a test with the libpkix verification code path.
Unfortunately that produces different results:

  kuix.de:9450 uses an invalid security certificate.
  The certificate is not trusted because it was issued by 
      an invalid CA certificate.
  (Error code: sec_error_ca_cert_invalid)

This error is overridable.


I think we should use a patch that produces the same error code with both verification engines.

Comment 4

[Wan-Teh Chang]

Kai, could you repeat your libpkix test without my patch?
I am wondering if libpkix doesn't like your root CA cert
for some reason.

Comment 5

[Wan-Teh Chang]

Attachment: MD5 server certificate of https://prefix.pch.net/

https://prefix.pch.net/ has an MD5 certificate that expires
June ‎29, ‎2013.  (There is no intermediate CA certificate in
the certificate chain.)  We can use this certificate for
testing.

Comment 6

[Wan-Teh Chang]

Attachment: Patch v2

I fixed the problem with libpkix my patch.  Please review and
test it.

The remaining issue is to pick an error code name.  The
current name SEC_ERROR_ALGORITHM_DISABLED_BY_POLICY is clear
and can be used for all algorithms that are disabled by
policy.  But it is useful for the error page to point out
it is the signature algorithm that has an error.  I am afraid
that SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED_BY_POLICY is too
long.  We can consider:
  SEC_ERROR_ALGORITHM_DISABLED_BY_POLICY (name used in the patch)
  SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED
  SEC_ERROR_DISABLED_SIGNATURE_ALGORITHM
  SEC_ERROR_BLOCKED_SIGNATURE_ALGORITHM
  SEC_ERROR_WEAK_SIGNATURE_ALGORITHM

What is your preference?

Comment 7

[Kai Engert (:KaiE:)]

How about shortening?

  SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED_BY_POLICY

  => SEC_ERROR_SIG_ALG_DISABLED_BY_POLICY

Comment 8

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #3)
> In addition I performed a test with the libpkix verification code path.
> Unfortunately that produces different results:
>   (Error code: sec_error_ca_cert_invalid)

Sorry, I made a mistake. My test root was untrusted.

Just for completeness, I repeated the test with your 
FIRST (obsolete) patch with both sites (a) kuix (b) pch

a + classic: disabled_by_policy
b + classic: disabled_by_policy
a + pkix: bad_signature
b + pkix: bad_signature


I'll test your new patch next.

Comment 9

[Kai Engert (:KaiE:)]

Comment on attachment 609986 [details] [diff] [review]
Patch v2

This patch behaves correctly.

We get the new error "disabled_by_policy" for both sites, with both classic and libpkix verification engines.

Comment 10

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #6)
> 
> The remaining issue is to pick an error code name.  The
> current name SEC_ERROR_ALGORITHM_DISABLED_BY_POLICY is clear
> and can be used for all algorithms that are disabled by
> policy.  But it is useful for the error page to point out
> it is the signature algorithm that has an error.  I am afraid
> that SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED_BY_POLICY is too
> long.

I think it would be good if the error code hinted to all of the following:
- disabled
- algorithm in signature
  (rather than just an algorithem)

In my opinion it is OK for the error code to use abbreviations.
We already use the abbreviations SIG in other error codes, examples:
- SEC_ERROR_OCSP_REQUEST_NEEDS_SIG 
- SEC_ERROR_PKCS7_KEYALG_MISMATCH


My preference is error code SEC_ERROR_SIG_ALG_DISABLED_BY_POLICY.

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 609986 [details] [diff] [review]
Patch v2

At the functional level this patch looks good to me, r=kaie

However, I'll mark it as r-, because we should at least use a
more detailed wording for the error code description.

I propose to also change the error code to:
  SEC_ERROR_SIG_ALG_DISABLED_BY_POLICY

Instead of
  "Algorithm disabled by policy."
I propose:
  "Rejecting a signature because it uses an algorithm that has been disabled by policy."

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Wan-Teh, this is your patch with the error code/description change I proposed.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Kai Engert (:kaie) from comment #11)
> I propose:
>   "Rejecting a signature because it uses an algorithm that has been disabled
> by policy."

If this would ever be shown to end-users, especially in the Firefox UI, then this wording would be better:

"The certificate was signed using an signature algorithm that is disabled because it is insecure."

Reasons this wording is better:

* It is a complete sentence.

* It indicates that the algorithm is insecure, so the user shouldn't try to enable that signature algorithm to fix the problem.

* It removes "by policy." Windows users are likely to mis-interpret this as referring to Windows Group/Local security policy mechanism. More generally, it isn't clear who has made the policy decision.

Comment 14

[Wan-Teh Chang]

Kai, Brian: thank you for your comments.  I agree "by policy"
is ambiguous.  (I used it because of the NSS function name
NSS_GetAlgorithmPolicy, which is involved in this error.)

Let's go with SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED and the
error message Brian suggested.  (It is only appropriate for
verifying certificate signatures, so we'd need a different
error message when we use this error code in libsmime.)

Microsoft Manual of Style recommends using "not secure" instead
of "insecure".

Comment 15

[Robert Relyea]

Comment on attachment 610081 [details] [diff] [review]
Patch v3

This patch is just the error strings, which looks like there were further discussions since the patch was put out.

the non-error string part of wtc's patch 2 look fine to me. I'm also ok with the error code without the word policy (though Policy in the error code is less likely to be confusing since the users who see the actual error code are usually programmers).

I'm fine with Brian's suggested wording for the external error strings.

I would r+ a patch with features.

bob

Comment 16

[Kai Engert (:KaiE:)]

(In reply to Brian Smith (:bsmith) from comment #13)
> 
> "The certificate was signed using an signature algorithm that is disabled
> because it is insecure."

I disagree with this wording, because it uses the term "certificates",
but the error code is not limited to certificates,
but general purpose for signatures.

I propose the following variation:

"The digital signature uses an algorithm that is disabled because it is insecure."

Comment 17

[Kai Engert (:KaiE:)]

(In reply to Wan-Teh Chang from comment #14)
> 
> Microsoft Manual of Style recommends using "not secure" instead
> of "insecure".


Ok, let's use:

"The digital signature uses an algorithm that is disabled because it is not secure."

Comment 18

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #15)
> 
> This patch is just the error strings, which looks like there were further
> discussions since the patch was put out.

My mistake, I attached the wrong patch.

Comment 19

[Kai Engert (:KaiE:)]

Attachment: Patch v5

This patch v2 is functionally equivalent to v2, and I give it r=kaie
for the functionality.

The patch uses the new error code
  SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED
that wan-Teh proposed in comment 14.
Nobody has objected this name, I agree with it,
I made the change on behalf of Wan-Teh,
and I give r=kaie for this error code.

This patch uses the string that Brian proposed in comment 13,
but with the change that Wan-Teh proposed in comment 14,
and with the change that I proposed in comment 16 - 17:

"The digital signature uses an algorithm that is disabled because it is not secure."

Comment 20

[Kai Engert (:KaiE:)]

Comment on attachment 616512 [details] [diff] [review]
Patch v5

Wan-Teh, can you please review the wording

  "The digital signature uses an algorithm that is disabled because 
   it is not secure."

in this patch?

Comment 21

[Kai Engert (:KaiE:)]

During the NSS conference call, Brian and Wan-Teh agreed with the wording.

Also, Wan-Teh had proposed that he might check more deeply where this error code is being used, in particular, whether the error code might be used in the S/MIME signature scenario, too. I'm afraid he didn't have the time yet to work on that. Given the deadline of tomorrow, we need to act now, and I propose we don't wait for that checks.

Let's go ahead with this wording as is, because having this error code and message is an improvement.

Comment 22

[Kai Engert (:KaiE:)]

Based on the generic wording of the error code, I had incorrectly concluded that the error code would be generic for any kind of signature checking.

But it appears that Wan-Teh's patch only modifies the certificate checking code.

Because of this, I agree with Brian's wording, which Wan-Teh has agreed to.
I'll attach a new patch.

Comment 23

[Kai Engert (:KaiE:)]

I propose to add CERT_ to the error code, to make it clear this error code and error string is specific to certificates. An S/MIME specific error code should be introduced if desired.

The new error code would be SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED

The length shouldn't be an issue, we already have even longer error codes.

Comment 24

[Kai Engert (:KaiE:)]

Attachment: Patch v6

Comment 25

[Kai Engert (:KaiE:)]

Comment on attachment 617709 [details] [diff] [review]
Patch v6

r=kaie on Wan-Teh's patch v2

I updated the patch to use the string that Brian and Wan-Teh discussed in comment 13 and 14. r=kaie on that string.

I changed the error code to SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED

Comment 26

[Kai Engert (:KaiE:)]

Checked in to cvs trunk.

Checking in lib/certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.76; previous revision: 1.75
done
Checking in lib/libpkix/include/pkix_errorstrings.h;
/cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h,v  <--  pkix_errorstrings.h
new revision: 1.38; previous revision: 1.37
done
Checking in lib/libpkix/pkix/checker/pkix_signaturechecker.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/checker/pkix_signaturechecker.c,v  <--  pkix_signaturechecker.c
new revision: 1.2; previous revision: 1.1
done
Checking in lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c,v  <--  pkix_pl_cert.c
new revision: 1.29; previous revision: 1.28
done
Checking in lib/libpkix/pkix_pl_nss/pki/pkix_pl_crl.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_crl.c,v  <--  pkix_pl_crl.c
new revision: 1.15; previous revision: 1.14
done
Checking in lib/util/SECerrs.h;
/cvsroot/mozilla/security/nss/lib/util/SECerrs.h,v  <--  SECerrs.h
new revision: 1.23; previous revision: 1.22
done
Checking in lib/util/secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.30; previous revision: 1.29
done

Comment 27

[Kai Engert (:KaiE:)]

Checked in to NSS 3.13.4 branch for NSS 3.13.5

Checking in certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.75.2.1; previous revision: 1.75
done
Checking in libpkix/include/pkix_errorstrings.h;
/cvsroot/mozilla/security/nss/lib/libpkix/include/pkix_errorstrings.h,v  <--  pkix_errorstrings.h
new revision: 1.37.2.1; previous revision: 1.37
done
Checking in libpkix/pkix/checker/pkix_signaturechecker.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix/checker/pkix_signaturechecker.c,v  <--  pkix_signaturechecker.c
new revision: 1.1.36.1; previous revision: 1.1
done
Checking in libpkix/pkix_pl_nss/pki/pkix_pl_cert.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_cert.c,v  <--  pkix_pl_cert.c
new revision: 1.28.2.1; previous revision: 1.28
done
Checking in libpkix/pkix_pl_nss/pki/pkix_pl_crl.c;
/cvsroot/mozilla/security/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_crl.c,v  <--  pkix_pl_crl.c
new revision: 1.14.6.1; previous revision: 1.14
done
Checking in util/SECerrs.h;
/cvsroot/mozilla/security/nss/lib/util/SECerrs.h,v  <--  SECerrs.h
new revision: 1.22.2.1; previous revision: 1.22
done
Checking in util/secerr.h;
/cvsroot/mozilla/security/nss/lib/util/secerr.h,v  <--  secerr.h
new revision: 1.29.2.1; previous revision: 1.29
done

Comment 28

[Daniel C]

"an signature algorithm" should be "a signature algorithm". This also appears in bug 738457 and bug 738458.

Comment 29

[Wan-Teh Chang]

Attachment: Fix typo (an => a) in the error message

djcater: thank you for reporting the typo.  It used to be
"an algorithm".  The typo was introduced when we added
"signature" in the middle.

Patch checked in on the NSS trunk (NSS 3.14) and the
NSS_3_13_4_BRANCH (NSS 3.13.5).

Checking in SECerrs.h;
/cvsroot/mozilla/security/nss/lib/util/SECerrs.h,v  <--  SECerrs.h
new revision: 1.25; previous revision: 1.24
done

Checking in SECerrs.h;
/cvsroot/mozilla/security/nss/lib/util/SECerrs.h,v  <--  SECerrs.h
new revision: 1.22.2.2; previous revision: 1.22.2.1
done