Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does checkAccessModeAdmin() handle ACCESS_MODE_ALWAYS correctly? #70

Open
spaikmos opened this issue Nov 13, 2023 · 9 comments
Open

Does checkAccessModeAdmin() handle ACCESS_MODE_ALWAYS correctly? #70

spaikmos opened this issue Nov 13, 2023 · 9 comments

Comments

@spaikmos
Copy link
Contributor

In PIVSecurityProvider.java, lines 307-314 of checkAccessModeAdmin():

    //
    // ACCESS CONDITION 3 - User Administration Privilege
    //
    if ((mode != PIVObject.ACCESS_MODE_ALWAYS)
        && ((mode & PIVObject.ACCESS_MODE_USER_ADMIN) == PIVObject.ACCESS_MODE_USER_ADMIN)
        && checkAccessModeObject(object)) {
      result = true;
    }

This checks that ACCESS_MODE_USER_ADMIN is set when ACCESS_MODE_ALWAYS is not specified. There is no clause that results TRUE when ACCESS_MODE_ALWAYS is set. This means that an object with ACCESS_MODE_ALWAYS will not pass the security check in this method.

Should there be another if() clause for this? e.g.

    if(mode == PIVObject.ACCESS_MODE_ALWAYS) {
      result = true;
    }

Or is this specifically not allowed for the checkAccessModeAdmin() function? I see that it is handled as I would expect in the checkAccessModeObject() method.

The code path that led me to this is from PIV.putData(). I expect that I can call putData() on a PIV object with ACCESS_MODE_ALWAYS and be able to mutate it, but the code is preventing that. Is this a bug or WAI?

@makinako
Copy link
Owner

makinako commented Nov 14, 2023

Hi you are correct, this was a very early decision not to permit ACCESS_MODE_ALWAYS to be used for administrative functions. This was later relaxed to include the ACCESS_MODE_USER_ADMIN but I note that this wasn't documented.

So on to the discussion about whether it should exist. The main argument for it is of course flexibility. In some circumstances it may be useful to allow this functionality.

The arguments against are:

  1. Modification of any SSP (cryptographic operational data like keys, pins, certificates, etc) is explicitly not permitted by FIPS 140-3. This does not apply to operational data (i.e. any data not used for a cryptographic operation), but there is no practical way to describe whether a file is a cert or just some business rules and keys are obviously always cryptographic.
  2. It is too easy to manipulate this into a variety of attacks (though obviously if you are configuring this for a particular file or key you are hopefully thinking about how to protect it, but it violates the "Stop people from configuring the system in a bad way, even accidentally" principle.
  3. There is no audit trail on the card, for reasons that may or may not be self-evident anymore given the progress of high capacity cards, but without some kind of auth there is no trail back to who made the change, or when / where.
  4. It is easy to perform a DoS by over-allocating (a hard limit would solve this)

For reason 1 alone, we excluded it. However I have been in the process of making a FIPS_APPROVED boolean const that guarantees certain functionality is not available in the FIPS version, so it is worth a discussion about the use case, but it would need to be excluded in the FIPS version or the cert is basically unattainable.

@spaikmos
Copy link
Contributor Author

spaikmos commented Nov 14, 2023

I see, thanks for the clarification.

It makes sense that you would not want this enabled for a FIPS version. In our use case, we're adding extra features to the PIV credential. We could write separate applets on the card, but that would not be as convenient since the credential has everything else we want (authentication, CHUID, etc).

As it is currently implemented, I don't think ACCESS_MODE_USER_ADMIN is working. Looking further at the checkAccessModeAdmin() snippet referenced above, I see that the checkAccessModeObject() call only returns TRUE if the mode is ACCESS_MODE_ALWAYS (which must be false per line 310) or the PIN is validated (which defeats the purpose of ACCESS_MODE_USER_ADMIN). So it seems there are two discussions to be had:

  1. Whether an ACCESS_MODE_USER_ADMIN should exist at all.
  2. If it should exist, how to fix the current implementation?

For the FIPS_APPROVED boolean, would it be possible to hardcode the access mode for FIPS objects only, and then allow any other object ID to be user configurable? Or does the approval require that NO OBJECTS can be modified in this manner?

As for fixing the implementation, I think a couple things may need to changed:

a) The logic of checkAccessModeAdmin() needs to be modified to allow ACCESS_MODE_USER_ADMIN to work properly.
b) To do this, it may require ACCESS_MODE_USER_ADMIN to be defined as 0x80 instead of 0x10, so that it may be combined with ACCESS_MODE_ALWAYS (0x7F) so a user can ask for both.

The reason I say this is because I want to be able to putData() and getData() some objects without having to do any extra authentication or using a PIN. I know this is not secure and easy to abuse, but it's for data that is not sensitive. We can protect the data by appending a signed hash if desired.

FWIW, I had implemented this in a different way:

I added a new attribute, RW_MODE, to the PIV Object. It can be RW_MODE_READ_ONLY or RW_MODE_READ_WRITE and is configured via the putDataAdmin(). In addition, if the RW_MODE is not specified, then the applet defaults to RW_MODE_READ_ONLY. So it is backwards compatible with previous provisioning scripts. This wasn't used for key generation tho.

One thing I considered (but didn't address) is whether to limit the size of the data that the user writes to the card in this mode. E.g. set a limit to prevent an attacker from writing large amounts of data to fill up the card. This would need to be addressed in some manner; perhaps define another parameter like MAX_OBJECT_SIZE to constrain the size of these USER_ADMIN-able objects.

@EmperorArthur
Copy link

Random person here, who has not looked at the code. I like the idea of RW_MODE.

That's how real file systems deal with this problem. It makes it more likely FIPS mode won't be utter garbage,* and allows some additional fine grained per object permissions.

Something like this.

  • 0x00 Read Only
  • 0x01 Read requires pin
  • 0x02 User Write allowed
  • 0x04 Admin Write allowed
  • 0x08 Write requires pin

* From a user standpoint FIPS is such a pain. It's only used when required by an outside regulation or contract. The number of things that break in FIPS mode on machines even when they shouldn't is annoying.

@dengert
Copy link

dengert commented Nov 14, 2023

The reason I say this is because I want to be able to putData() and getData() some objects without having to do any extra authentication or using a PIN. I know this is not secure and easy to abuse, but it's for data that is not sensitive. We can protect the data by appending a signed hash if desired.

The putData() without any authentication sounds like a big security problem. I would assume the data is associated with the PIV data on the card, or you would not need to write it. Can you elaborate on what type of data you would write to the card?

https://pivkey.com/ has support for: "The PIVKey C70 Series of cards supports Proximity (125kHz) in multiple data formats, as well as MIFARE DESFire®." I have not checked what it takes to write this data, but I would bet it requires some authentication.

@spaikmos
Copy link
Contributor Author

The putData() without any authentication sounds like a big security problem. I would assume the data is associated with the PIV data on the card, or you would not need to write it. Can you elaborate on what type of data you would write to the card?

I agree, putData() without authentication can be abused. It is appropriate for some data that we want to make configurable. The main thing we want is binding this data to the PIV identity.

Our application uses the PIV credential as an employee badge for corporate physical access. At the company, there are use cases for tapping your badge to access shared resources.

A contrive example: an employee taps their badge on a reader to get into a queue for on-site tech support. The employee's computer details (linux/windows/mac, asset number, etc) are stored on the badge as a separate PIV object, and the tech support folks can update the information as they issue new hardware to the employee. The data could be protected with a signature if necessary to verify authenticity, but it wouldn't protect the data from being overwritten by a malicious actor. And we would be ok with that.

@dengert
Copy link

dengert commented Nov 14, 2023

That sounds like PIV CHUID authentication. The CHUID can have a GUID, and or a FASC-N (i.e. a user id) and non gov FASC-N start with 99999.

NIST PIV docs talk about physical access, and the CHUID authentication is still allowed, but draft sp800-73-5 and FIPS-201-5 are dropping in. https://csrc.nist.gov/pubs/sp/800/73/pt1/5/ipd

But you say: "the tech support folks can update the information as they issue new hardware to the employee." Are these the same people that issue the badges? If not sounds like that should have to use some pin to authenticate to the card to write the new data.

But it also sounds like the data is for authorization not authentication and changes more often then a new card. Most systems authenticate the user and then consult some authorization database. i.e. "binding this data to the PIV identity."

If you stick with standard PIV you don't need the extra changes to to the PIV applet and a malicious actor has no data to overwrite.

Our application uses the PIV credential as an employee badge for corporate physical access. At the company, there are use cases for tapping your badge to access shared resources.

You can also use the PIV for logical access to Windows AD too and with some linux PAM modules you can use AD as a KDC.

@spaikmos
Copy link
Contributor Author

We're not doing CHUID auth, or any auth for that matter.

This is a non-PIV use case. I guess the proper term is CIV? For our project, we are using the PIV capabilities for access control, and then adding extra (non-auth) functionality on top of the same credential. This is why we'd like the ability to read/write objects.

PIN auth is too high friction for what we're doing. Essentially, we want to store little bits of data on the employee's badge so they can be used by offline systems. We're ok with malicious actors being able to manipulate the data, similar to how most companies are fine with using PROX for access control.

@dengert
Copy link

dengert commented Nov 21, 2023

Are you developing this for internal only use or are you developing a product?
What do you mean by: "we are using the PIV capabilities for access control"? Are you referring to physical access such as door locks?
Do you have this working?
Without using a PIN?

Other than CHUID authentication the only real authentication that can be used without a PIN is the PIV 9E key that can authenticate the card, but only possession of the card and not the user.

In regards to CIV - A search for: PIV vs CIV leads to:
https://www.securetechalliance.org/publications-a-comparison-of-piv-piv-i-and-civ-credentials/

You are asking for changes to the Makinako PIV applet, which is trying to follow the NIST FIPS standards. But are asking for changes that would not be allowed by these standards.

Have you looked at other PIV like cards on the market? For example the PIVKey cards. https://pivkey.com/
https://shop.pivkey.com/PIVKey-smart-cards/ says the C990 has PIV plus Desfire $19.95 per card.

There are also Yubikey tokens https://www.yubico.com/ that implement PIV, OpenPGP, FIDO.

I am not associated with makinako, but am the author of the PIV driver for OpenSC. https://github.com/OpenSC/OpenSC
This is used with certificated PIV cards, Yubikey, PIVKey and https://github.com/arekinath/PivApplet which is used in testing OpenSC.

@makinako
Copy link
Owner

For my $0.02, it is essential that OpenFIPS201 can say that it complies with the PIV standard, however we achieve this by configuration and our current level of flexibility already does this in a number of areas that would otherwise be non-compliant.

We're generally cautious about adding functionality that significantly changes the goal of "A more flexible version of the PIV standard", but on reflection the kind of change @spaikmos is talking about would not be considered outside of this because:

  1. It can be included or not by configuration
  2. The configuration element is already there, it just needs to be adhered to for administration as well as reading
  3. There is an argument for its use and a practical case that a user of OpenFIPS201 can see (For something that is already so close to what it can do, I would rather see the functionality in the common code base rather than forking to achieve it)

It would need to be excluded from the FIPS build explicitly as I think I mentioned earlier. But that's fairly easy to do any prove.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants