1258359 - lg_FindTrustAttribute can't handle all trust attributes

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Franziskus Kiefer [:franziskus]]

When bug 1224139 lands and PK11_GetAttributes return values are checked merge.sh tests fail if executed within all.sh.

It looks like the tests are either broken and shouldn't be called this way, or PK11_GetAttributes misbehaves and should work with the given values.

Comment 1

[Franziskus Kiefer [:franziskus]]

After having some time to look at this I came to the following conclusion. Not sure how to fix it though.

In lgattr.c:lg_FindTrustAttribute the first |switch (type)| is missing CKA_TRUST_IPSEC_END_SYSTEM, CKA_TRUST_IPSEC_TUNNEL, CKA_TRUST_IPSEC_USER, and CKA_TRUST_TIME_STAMPING and thus fails the merge tests (with bug 1224139 landed that actually checks return values of PK11_GetAttributes). This is easy to fix by adding them to the switch statement.
However, the second switch statement that sets |trustFlags| has to actually copy the trust flags and I'm not sure under which conditions they're supposed to be set.

Comment 2

[Kai Engert [:KaiE:]]

I've never looked at that code before, and your comments aren't straightforward to understand, so all I can do is to try to help with brainstorming.

You haven't explained WHY you think that CKA_TRUST_IPSEC_END_SYSTEM and the others you have mentioned are missing.

So apparently function lg_FindTrustAttribute knows how to extract trust information from some attribute types, and filters out all types that it doesn't know about in the first switch.

I think you're saying, you see that attribute types are being used, which aren't handled in lg_FindTrustAttribute, and an "invalid attribute" is returned, with a failure result. The failure result is ignored, and the outside code proceeds, attempting to process it.

I don't know the architecture behind that. Maybe all the trust attribute processing is lenient and will ignore if invalid attributes have been returned?

It would require someone with detailed knowledge to explain us if and why it's safe that currently those failure return values are ignored.

I hope that Bob can explain it.

Comment 3

[Robert Relyea]

Kai has it right. There is no reason to add support for CKA_TRUST_IPSEC_XXX into the legacy database. I believe we've switch the sql by default. At some point in the near future we should stop compiling the legacy database by default.

If failures in PK11_GetAttributes() is actually causing test failures, then the tests need to be updated. It's perfectly legal for PKCS11_GetAttributes to fail, particularly on CKA_TRUST attributes, which are NSS vendor specific attributes anyway.