Closed Bug 738454 Opened 8 years ago Closed 8 years ago

Introduce a separate error code SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED

Categories

(NSS :: Libraries, defect, P1)

3.12.3
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.13.5

People

(Reporter: KaiE, Assigned: wtc)

References

Details

Attachments

(3 files, 4 obsolete files)

Attached patch Patch v1 (from Wan-Teh) (obsolete) — Splinter Review
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.
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.
Blocks: 738457
Blocks: 738458
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?
Attachment #608466 - Flags: review?
Attachment #608466 - Flags: feedback+
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.
Attachment #608466 - Flags: feedback+ → feedback-
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.
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.
Assignee: nobody → wtc
Status: NEW → ASSIGNED
Priority: -- → P1
Version: 3.13.3 → 3.12.3
Attached patch Patch v2 (obsolete) — Splinter Review
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?
Attachment #608466 - Attachment is obsolete: true
Attachment #608466 - Flags: review?
Attachment #609986 - Flags: superreview?(rrelyea)
Attachment #609986 - Flags: review?(kaie)
How about shortening?

  SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED_BY_POLICY

  => SEC_ERROR_SIG_ALG_DISABLED_BY_POLICY
(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 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.
(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 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."
Attachment #609986 - Flags: superreview?(rrelyea)
Attachment #609986 - Flags: review?(kaie)
Attachment #609986 - Flags: review-
Attached patch Patch v3 (obsolete) — Splinter Review
Wan-Teh, this is your patch with the error code/description change I proposed.
Attachment #610081 - Flags: superreview?(rrelyea)
Attachment #610081 - Flags: review?(wtc)
(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.
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 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
Attachment #610081 - Flags: superreview?(rrelyea)
Target Milestone: 3.13.4 → 3.13.5
(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."
(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."
(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.
Attachment #610081 - Attachment is obsolete: true
Attachment #610081 - Flags: review?(wtc)
Attached patch Patch v5 (obsolete) — Splinter Review
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."
Attachment #609986 - Attachment is obsolete: true
Attachment #616512 - Flags: superreview?(wtc)
Attachment #616512 - Flags: review+
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?
No longer depends on: 590364
Summary: Introduce a separate error code for bad-signature-because-algorithm-disabled-by-policy → Introduce a separate error code SEC_ERROR_SIGNATURE_ALGORITHM_DISABLED
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.
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.
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.
Attached patch Patch v6Splinter Review
Attachment #616512 - Attachment is obsolete: true
Attachment #616512 - Flags: superreview?(wtc)
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
Attachment #617709 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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
"an signature algorithm" should be "a signature algorithm". This also appears in bug 738457 and bug 738458.
Blocks: 758314
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
You need to log in before you can comment on or make changes to this bug.