mainGnuTLS - Support: sr #107489, ipsec_ike_key created in wrong...

 
 

sr #107489: ipsec_ike_key created in wrong code path

Submitter:  Micah Anderson <micahanderson>
Submitted:  Sat 02 Oct 2010 01:36:39 PM UTC
   
 
Category:  None Priority:  5 - Normal
Severity:  3 - Normal Status:  In Progress
Privacy:  Public Assigned to:  nmav
Open/Closed:  Closed Operating System:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 08 Oct 2010 06:37:15 AM UTC, comment #9: 

I've checked again the patch and I think the one without the last fix is quite better. That is because you call functions to verify values such as get_sign_status (get_tls_server_status()) that work pretty ok in the batch mode, but have issues in the interactive mode (such as asking the same questions again and again).

Nikos Mavrogiannopoulos <nmav>
Group administrator
Thu 07 Oct 2010 07:26:40 AM UTC, comment #8: 

In line:
if (get_ipsec_ike_status && (get_sign_status (get_tls_server_status()) !=1))

You check for get_ipsec_ike_status which is always non-zero since it is a pointer to a function and the patch does not apply cleanly to 2.8.x branch. Probably you meant to check for something else there. Could you create a test script similar to the existing "sha2" that will check the expected behavior? This will make it much easier to verify that everything works as expected.

Nikos Mavrogiannopoulos <nmav>
Group administrator
Tue 05 Oct 2010 01:13:55 PM UTC, comment #7: 

Originally, the patches submitted in #107485 only created a certificate with the KP flag for ipsec IKE only when the "ca" flag was set. That was because the get_ipsec_ike_status() check in src/certtool.c:547 was wrapped inside an if (ca_status) predicate. That was the incorrect place to have that check, as the ipsec_ike_key KP should not be set in a certificate designated for a CA.

If you compile the code, with the patches, you can test that the following are true:

1. build a cert with ipsec_ike_key set in the template, it should create a cert with the following:

Key Purpose (not critical):
Ipsec IKE.

this test checks to make sure that you get a certificate built with the Ipsec IKE KP set, when there is no 'ca' flag set. This was the original code pathing problem.

2. build a cert with the 'ca' option set in the template, along with the ipsec_ike_key option set. It should have the following v3 extension Basic Constraint set, and no Ipsec IKE Key Purpose set:

Basic Constraints (critical):
Certificate Authority (CA): TRUE

This test checks to make sure that when you set the 'ca' flag, no Ipsec IKE KP is set, as this KP should not be set when the certificate is a CA certificate. This is the inverse of the previous test.

3. build a certificate with ipsec_ike_key set in the template, with multiple dns_name parameters set (dns_name="test.gnutls.org, test2.gnutls.org") and an IP address set (ip_address=192.168.1.1) to achieve a certificate with subjectAltNames set. The certificate should have the Key Purpose Ipsec IKE set as in test #1 above, and the SubjectAltName bits should be set:

Subject Alternative Name (not critical):
DNSname: test.gnutls.org, test2.gnutls.org
IPAddress: 192.168.1.1

This test makes sure that subjectAltName support works with the Ipsec IKE KP.

4. build a certificate with "tls_www_server" set along with "ipsec_ike_key" set and subjectAltNames as in test #3. The generated certificate Key Purpose should have both "TLS WWW Server." set as well as "Ipsec IKE." and the subjectAltNames should also be set, as in test #3

This test ensures that you can build a certificate with multiple KP's set, along with Ipsec IKE. With the added bonus that the subjectAltNames should continue to persist.

5. build a certificate with "signing_key" set, as well as "ipsec_ike_key" set. This should result in a certificate that has the "Ipsec IKE" Key Purpose set, and the Key Usage set to "Digital Signature", as follows:

Key Purpose (not critical):
Ipsec IKE.
Key Usage (critical):
Digital signature.

This test makes sure that when the Ipsec IKE KP is set, and 'signing_key' is set, that the KU is properly set to Digital signature. This tests compliance with RFC 4945 § 5.1.3.2.

6. build a certificate with 'encryption_key' set, along with 'ipsec_ike_key' set. This should result in a certificate that has the "Ipsec IKE" Key Purpose set, and the Key Usage set to both "Non repudiation" and "Key encipherment", as follows:

Key Purpose (not critical):
Ipsec IKE.
Key Usage (critical):
Non repudiation.
Key encipherment.

This test makes sure that when Ipsec IKE KP is set, and encryption_key is set, that the Non repudiation KU is also set. This tests compliance with RFC 4945 § 5.1.3.2.

7. build a certificate with 'encryption_key' set, along with 'signing_key' set, and with 'ipsec_ike_key' set. This should result in a certificate that has the "Ipsec IKE" Key Purpose set, and the Key Usage flags set to both "Digital Signature" and "Key encipherment" as follows:

Key Purpose (not critical):
Ipsec IKE.
Key Usage (critical):
Digital signature.
Key encipherment.

This test makes sure that when Ipsec IKE KP is set, and both encryption_key and signing_key are set, that the KU is properly set to have both Digital signature and Key encipherment (and not Non Repudiation). This tests compliance with RFC 4945 § 5.1.3.2.

Micah Anderson <micahanderson>
Tue 05 Oct 2010 06:55:17 AM UTC, comment #6: 

Hi,
 Could you post information on which should have been the "correct" path for the code, so we can verify it as well?

Anonymous
Mon 04 Oct 2010 05:35:19 PM UTC, comment #5: 

I should note that you can also find these commits in my repository which is located here: git://labs.riseup.net/~micah/gnutls there are two branches there, one for 2_10_x and one against master (which has been rebased against the latest upstream commits).

Micah Anderson <micahanderson>
Mon 04 Oct 2010 05:28:29 PM UTC, comment #4: 

Ok, I have done some additional testing to verify things work as expected. As a result, I found that the certificate request code path needed to also be adjusted. I've attached those patches, and committed them to my git repository.

I was able to test generation of certificates and generation of certificate requests, and then also generating certificates based on those requests. I set various flags for different tests, and am now satisfied that it is doing the correct thing.


(file #21611, file #21612)

Micah Anderson <micahanderson>
Sat 02 Oct 2010 09:34:59 PM UTC, comment #3: 

I've applied the patches to 2.10.x and master. Could you verify that they work as expected?


Nikos Mavrogiannopoulos <nmav>
Group administrator
Sat 02 Oct 2010 06:34:53 PM UTC, comment #2: 

the attached patches add the section 5.1.3.2 compliance on top of the other attached patches. also available in my repository

(file #21596, file #21597)

Micah Anderson <micahanderson>
Sat 02 Oct 2010 02:01:56 PM UTC, comment #1: 

There is another issue with the ike patch that must be resolved:

according to http://tools.ietf.org/html/rfc4945#section-5.1.3.2:

if any KU is set,
then either digSig or nonRep must be set within that KU for all IKE certs

this is currently not happening in the attached patches, so please hold off on applying these until this has been adjusted.

Micah Anderson <micahanderson>
Sat 02 Oct 2010 01:36:39 PM UTC, original submission:  

The ipsec_ike_key patch submitted in #107485 creates a certificate with the KU flag for ipsec IKE only when the "ca" flag is set. The reason for this is the get_ipsec_ike_status() check in src/certtool.c:547 is wrapped inside an if (ca_status) predicate.

This is wrong, because you should not be a CA to offer a cert for IKE. In fact, IKE should not appear in a CA certificate, but otherwise should be independent of any other status as it is not unreasonable to want to use such a certificate for other things on the host, such as a WWW server.

It is for this reason I've adjusted the patch to make it fall under the if (!ca_status || server) predicate, instead of under the if (ca_status) predicate.

Additionally, an IKE certificate should be able to set the SubjectAltName (ie. dns_name parameters in the config; and ip_address parameters in the config) v3 extensions. To achieve this I've added an is_ike check and added the test to see if that is set along with the other checks that were happening, and then if so add the get_dns_name_set (TYPE_CRT, crt); get_ip_addr_set (TYPE_CRT, crt); to the cert.

Attached is a patch to the 2_10_x branch, as well as patch to the HEAD of master. You can also find these commits in my repository git://labs.riseup.net/~micah/gnutls there are two branches there, one for 2_10_x and one against master (which has been rebased against the latest upstream commits).

Micah Anderson <micahanderson>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by nmav (Posted a comment)
  • -email is unavailable- added by micahanderson (Submitted the item)
  • -email is unavailable- added by micahanderson
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-12-19 nmav Open/ClosedOpen Closed
    2010-10-04 micahanderson Attached File- Added ipsec_ike_crq_2_10_x.diff, #21611
        Attached File- Added ipsec_ike_crq_master.diff, #21612
    2010-10-02 nmav StatusNone In Progress
        Assigned toNone nmav
    2010-10-02 micahanderson Attached File- Added ipsec_ike_non_repudiation_2_10_x.diff, #21596
        Attached File- Added ipsec_ike_non_repudiation_master.diff, #21597
    2010-10-02 micahanderson Carbon-Copy- Added dkg
        Attached File- Added ipsec_ike_gnutls_2_10_2.diff, #21593
        Attached File- Added ipsec_ike_gnutls_master.diff, #21594

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code