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

Sensitive data is not audit-logged if read via calculated element #103

Closed
patricebender opened this issue Jun 20, 2024 · 3 comments
Closed
Labels
help wanted Extra attention is needed new security

Comments

@patricebender
Copy link
Member

patricebender commented Jun 20, 2024

it is possible to circumvent audit-logging of sensitive data, if the data is read via a calculated element.

I have created a sample which illustrates the problem. In the incidents-app, the credit card number of the customer is exposed in Incidents via calculated element.. In the sample, audit-logging has been added as well as the data-privacy annotations.

Steps to reproduce


→ Logs for direct read [odata] - GET /odata/v4/processor/Customers:

[audit-log] - SensitiveDataRead: {
  data_subject: {
    id: { ID: '1004100' },
    role: 'Customer',
    type: 'ProcessorService.Customers'
  },
  object: { type: 'ProcessorService.Customers', id: { ID: '1004100' } },
  attributes: [ { name: 'creditCardNo' } ],
  uuid: '6c05ad60-7dc8-41d0-bc1b-bd5fa7f549ba',
  tenant: 't1',
  user: 'alice',
  time: 2024-06-20T09:26:51.333Z
}

→ No logs for indirect read [odata] - GET /odata/v4/processor/Incidents, which exposes the credit card number of the customer:

{
  "@odata.context": "$metadata#Incidents",
  "value": [
    {
      "ID": "3583f982-d7df-4aad-ab26-301d4a157cd7",
      "createdAt": "2024-06-20T09:33:09.664Z",
      "createdBy": "anonymous",
      "modifiedAt": "2024-06-20T09:33:09.664Z",
      "modifiedBy": "anonymous",
      "customerCreditCardNo": "3456789012345678", // 😱
      "customer_ID": "1004100",
      "title": "Solar panel broken",
      "urgency_code": "H",
      "status_code": "I",
      "HasActiveEntity": false,
      "HasDraftEntity": false,
      "IsActiveEntity": true
    },
    
  ]
}
@patricebender patricebender added help wanted Extra attention is needed security labels Jun 20, 2024
@github-actions github-actions bot added the new label Jun 20, 2024
@sjvans
Copy link
Contributor

sjvans commented Jun 20, 2024

hi @patricebender

you'd need to annotate such calculated properties (and their carrying entities) explicitly

@patricebender
Copy link
Member Author

patricebender commented Jun 20, 2024

Okay, do you think it would make sense to document this somewhere? I was not aware of this and had expected it to work out of the box.

Also, the annotation is propagated to the calculated element, why not leverage this?

entity Authors {
    key ID         : Integer;
        books      : Association to many Books
                         on books.author = $self;
        creditCard : Int16 @PersonalData.IsPotentiallySensitive;
}

entity Books {
    key ID     : Integer;
        author : Association to Authors;
        title  : String;
        authorCreditCardNumber = author.creditCard;
}
 bin/cdsc.js e.cds
Debugger attached.
{
  "definitions": {
    "Authors": {
      "kind": "entity",
      "elements": {
        
        "creditCard": {
          "@PersonalData.IsPotentiallySensitive": true,
          "type": "cds.Int16"
        }
      }
    },
    "Books": {
      "kind": "entity",
      "elements": {
       
        "authorCreditCardNumber": {
          "@PersonalData.IsPotentiallySensitive": true, // ⬅️
          "type": "cds.Int16",
          "value": {
            "ref": [
              "author",
              "creditCard"
            ]
          }
        }
      }
    }
  },
  "meta": {
    "creator": "CDS Compiler v5.0.1"
  },
  "$version": "2.0"
}

@sjvans
Copy link
Contributor

sjvans commented Jul 10, 2024

it is documented in Annotating Personal Data. you cannot guess the entity-level annotation (e.g., personal data or personal data details?). hence, you need to add the annotations explicitly. repeating propagated annotations is not necessary, but i'd always annotate everything in the same location.

@sjvans sjvans closed this as completed Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed new security
Projects
None yet
Development

No branches or pull requests

2 participants