Sensitive Data in Server Response

[morarucostel]

Is your request related to a problem? Please describe. Sensitive information can be seen in some of the server responses. This can be dangerous if other vulnerabilities, such as Cross-Site Scripting, exist within the application. Furthermore, this information could be cached by the user's browser or any upstream proxy servers. This could potentially give attackers access to the sensitive information. Some organizations are running internal security analysis and it discovered the “secure” team parameters in plaintext within server responses.

For example, the following endpoints are returning in plaintext parameters that are marked as password, which typically represents sensitive data and they should be encrypted or hashed:

• flow/services/workflow/teams/{team-id}/properties
• flow/services/workflow/teams

Describe the solution you'd like The server response should encode or encrypt the fields marked as password type in order to hide the sensitive information. We can encode the parameter value before sending it as a response and the UI should be able to decode it before invoking the workflow.

Describe the benefits or justification for this request For some enterprise entities having the server responding with plain password like values is considered a security violation and will not allow the deployment of the component.

Additional context The functionality should not be impacted, the UI should still be able to invoke the execution of the workflow with the proper parameter value and to allow the user to update it if necessary.

[amhudson]

Currently, we store the secured/password value in plaintext in the db and return to the UI as is. There are two solutions here

1. Just encrypt the value/defaultValue of properties with "type":"password" that is returned in the response. The value/defaultValue remains in plaintext/decrypted in the db.

2. Encrypt the value/defaultValue of properties with "type":"password" end-to-end. Workflows using these properties for execution will need to decrypt the values.

In both cases, the UI will force a clear on edit of the field to prevent the user from modifying the encrypted value. The UI will send the value to the service in plaintext.

[tlawrie]

I thought one of the other implementations (cicd?) already implemented something around encrypted secured fields. Can someone maybe double check? we would then be able to levearge that.

On Fri, Apr 8, 2022 at 3:12 AM Adrienne Hudson @.***> wrote:

Currently, we store the secured/password value in plaintext in the db and return to the UI as is. There are two solutions here

1.

Just encrypt the value/defaultValue of properties with "type":"password" that is returned in the response. The value/defaultValue remains in plaintext/decrypted in the db. 2.

Encrypt the value/defaultValue of properties with "type":"password" end-to-end. Workflows using these properties for execution will need to decrypt the values.

In both cases, the UI will force a clear on edit of the field to prevent the user from modifying the encrypted value. The UI will send the value to the service in plaintext.

— Reply to this email directly, view it on GitHub https://github.com/boomerang-io/roadmap/issues/336#issuecomment-1091995407, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD5NMG7XMYGGURTJWU3XUB3VD4JOTANCNFSM5SWQXCYQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[amhudson]

yep, we do.

[tlawrie]

Do you have a rough idea of what the design is based on that or was that what you mentioned earlier?

On Sat, Apr 9, 2022 at 1:17 AM Adrienne Hudson @.***> wrote:

yep, we do.

— Reply to this email directly, view it on GitHub https://github.com/boomerang-io/roadmap/issues/336#issuecomment-1092989030, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD5NMG2N3LCWJZNDUPYU7H3VEBEZRANCNFSM5SWQXCYQ . You are receiving this because you commented.Message ID: @.***>

[morarucostel]

We had a call the other day together with @BenjaminRuby and one option is to decrease the surface of attack by not sending the value of the property at all (of type password) from the server to the client. The UI client doesn't show the password to the end-user so there is no real need to send to the ui client.

We can use a specific constant string like ***###*** instead of the real value in order to differentiate between 1/ the old and the updated value in the update parameter use-case and 2/ when execute the workflow and pass the value of the parameter use-case.

This way we eliminate the extra work to try and encrypt-decrypt the parameters, to send the key for the encryption/decription process, to secure the encryption key on the UI side.

[corcoja]

@morarucostel agree with your proposal with a few observations:

We had a call the other day together with @BenjaminRuby and one option is to decrease the surface of attack by not sending the value of the property at all (of type password) from the server to the client. The UI client doesn't show the password to the end-user so there is no real need to send to the ui client.

I would say this is definitely the way to go. There is no reason the send back the password to the FE since the BE already knows it.

We can use a specific constant string like ### instead of the real value in order to differentiate between 1/ the old and the updated value in the update parameter use-case and 2/ when execute the workflow and pass the value of the parameter use-case.

For 1. Instead of using a constant string like ***###***, we can change the endpoint (which updates a parameter) to use PATCH at its full potential. The endpoint is already of type PATCH, but I noticed it doesn't really omit the values that weren't updated, e. g. if I update just the description value, FE still puts all the fields inside the payload:

PATCH https://ocp2.cloud.boomerangplatform.net/dev/flow/services/workflow/workflow/612cd9468d2d9f32649b3f86/properties
[
  {
    "key": "my_pass",
    "label": "My Password",
    "description": "My super-duper secure password... not!",
    "required": false,
    "type": "password",
    "defaultValue": "can-you-see-me?",
    "jsonPath": "myPass"
  }
]

So instead of doing that, once the user updates certain fields, only those fields will be passed to the endpoint. If it happens to be the defaultValue of the password, then this will be updated. If not, then the default value that is stored inside the database just stay the same.

Examples: a. I want to update description and jsonPath:

[
  {
    "description": "Now my password is secure!",
    "jsonPath": "mySecurePass"
  }
]

b. I want to update defaultValue, required, and label:

[
  {
    "label": "My Very Secure Password",
    "required": true,
    "defaultValue": "you-ll-see-me-just-once",
  }
]

Now for 2., like we earlier discussed, there are three scenarios to cover when executing a workflow: the password parameter is not changed and the default value is used, the password parameter is changed but with an empty string, or the password parameter is changed with a non-empty string value.

For each case, the request can be updated to look like this (see my_pass param):

1. The password parameter is not changed and the default value is used (BE receives null and knows that it needs to use the default value for parameter my_pass when executing the workflow):

{
  "properties": {
    "my_pass": null,
    "some_text": "test",
    "some_int": 5,
    "another_param": true
  }
}

2. The password parameter is changed but with an empty string:

{
  "properties": {
    "my_pass": "",
    "some_text": "test",
    "some_int": 5,
    "another_param": true
  }
}

3. The password parameter is changed with a non-empty string value:

{
  "properties": {
    "my_pass": "s3cur3_p@55!",
    "some_text": "test",
    "some_int": 5,
    "another_param": true
  }
}

[morarucostel]

Hi @corcoja , I like the suggestions, I think we should go ahead with them.

[tlawrie]

@corcoja seconded.

I do think though that this will lead to some front end changes to really help explain this as well as documentation updates.

1. Update Team & Global Parameter Create Parameter modals by adding

   • Information about parameters generally
   • Helper text to the secure parameter toggle that explains you wont be able to edit it

2. Update Team & Global Parameter Edit Parameter modals with something like "Secret values are encrypted and cannot be displayed, but you can enter a new value". You will also have to make sure it shows a default number of ** in the field.

3. Adjust the Workflow Parameters screen modals as well. Potentially change the name of type 'Password' to 'Secured' or 'Secret'

4. Update the documentation to explain how secured / secret / password parameters are encrypted and cannot be displayed, but you can enter a replacement value.

Also do we want to encrypt them in the database?

[corcoja]

Hi @tlawrie,

I do think though that this will lead to some front end changes to really help explain this

For what I have suggested – definitely. Now, thinking more about this, I don't see a way that we could fix this issue without front end implication.

Also do we want to encrypt them in the database?

We haven't look into encryption at all so far. Our intent for this fix is to eliminate the clear-text "passwords" sent to the front end – this is priority.

[timrbula]

@BenjaminRuby to review comments

[BenjaminRuby]

Hey @corcoja @tlawrie I like the sound of this approach. I do agree with you Tyson that we will have to have some additional communication to the user about how their password is being stored and how they can update it, also how they can run their workflow with a default password (also open to potentially renaming the workflow parameter type from password -> secured)

[morarucostel]

Hey @AndreiOla and @BenjaminRuby , any progress with this one?

We need to have it in the 3.9.0 release (part of the security scan results).

[AndreiOla]

@morarucostel @tlawrie While I was making changes to the code, I noticed that for Teams, there is a different field with the password value. Is this one that should be filtered, or it is still the defaultValue? "name": "Test Team 2", "settings": { "properties": [ { "_id": "df5f5749-4d30-41c3-803e-56b54b768407", "value": "Value", "description": "this is the description", "key": "Key", "label": "asdf", "type": "password", "defaultValue": "sensitive password" -> I added this } ]

[morarucostel]

Hey @AndreiOla , I did a few tests and I can't see why for the Team parameters use-case we can have the defaultValue. The UI asks only for the value and it echos that field. The defaultValue is set to null, so I don't think you need to do anything on this field. Only on the value.

[tlawrie]

@morarucostel @AndreiOla firstly, I think, before we fully implement the server-side code we should change it to type secured instead of password as @BenjaminRuby mentioned. Both frontend and backend. And do a loader for data migration.

Secondly, as for the Team parameters, there is a toggle that should make them a secured value. The defaultValue field comes from the fact that all our user entered parameters should be using the same model but in this case its not something offered to the end-user.

[tlawrie]

And then we still need the UI updates to go in conjunction with this change before we can merge any backend PRs in.

[AndreiOla]

@tlawrie I agree that there are better solutions, but I would like to make the most of current design, that should go into current release (3.9.0) and fix this issue with smaller steps. And then plan the migration to a secured type of field, instead of password type, which involves more complex code changes. @morarucostel I will make changes to filter the value for Team->settings->properties.

[tlawrie]

@AndreiOla from point, from a services point of view that's fine. And create a new issue for the backend model change and data migration.

But as part of resolving this issue, we will still need some of the front end changes in 3.9.0

[AndreiOla]

@BenjaminRuby Should make the FE changes to send the password fields only if there is any new, valid value. Otherwise null/no value should be sent back to BE.

[AndreiOla]

Hey @BenjaminRuby Please treat this with priority, so we can include it in the Release.

[BenjaminRuby]

Hey @AndreiOla apologies on my slow response - I've been out of commission the last few days with the flu.

There is one scenario I think we need to consider on the UI. So we currently use the required boolean field on a workflow input parameter to drive validation in the form when running a workflow (i.e. if an input parameter is required - then the workflow can not be run until the user supplies a value).

What I want to do on the UI is set the required to false if the user provides a defaultValue (so that the user has the option w/ helper text to be able to run the workflow without supplying a value, thereby using the defaultValue in the database) - the only issue is that if the service returns null for the defaultValue, then the UI can not tell if there was a value originally set or not.

*this is only relevant for password/secure input types

@AndreiOla @corcoja thoughts on how you want to communicate that info to the UI? Do we need another boolean field?

[AndreiOla]

Hey @BenjaminRuby Right now the defaultValue (for Workflows) and value (for Teams) is returned as null. But I can set any String value, if there exists a value set in DB. So, null will be returned when there is no value set, and anything not null when there exists a value set. We can agree to return to the UI a specific String (e.g: valueset or just set) that would let the FE know there is a value set, so the UI can display ******** , or return empty String.

Also, please check Tyson's comment: https://github.com/boomerang-io/roadmap/issues/336#issuecomment-1096489433

[tlawrie]

Hi @BenjaminRuby im not quite understanding the need to change the required if there is a default value. Are you saying only for a secure value this would be a problem? Because the UI gets null back?

@AndreiOla the problem with returning a predefined value is collision.

Have we seen how forms of any kind do this in the web? Before we go ahead and

[tlawrie]

I suggest instead of returning null we return an encrypted value. Or we return a new element hidden: true|false

I am pretty sure UrbanCode Deploy used encrypted values. It used an encryption keystore. @gchickma any chance you remember the type of value it would return?

And Other tools in IBm have also used the hidden element

[AndreiOla]

I would not transfer any secured values (encrypted or not), if there is no need for them to be transferred. I don't quite follow the collision part. We should use a value that is definitely not a password. Maybe encrypt "set", or whatever value? This value is changed in the returned entities after the form data is saved into DB.

[BenjaminRuby]

Hi @BenjaminRuby im not quite understanding the need to change the required if there is a default value. Are you saying only for a secure value this would be a problem? Because the UI gets null back?

@AndreiOla the problem with returning a predefined value is collision.

Have we seen how forms of any kind do this in the web? Before we go ahead and

@tlawrie The approach we were going with was to be able to allow the user to not enter anything in order to run the workflow w/ their defaultValue (for password/secure input parameters).

The edge case for the UI in this approach comes if the input is specified as required and is type secure/password. Since we are getting a null defaultValue, we don't know if we can accept an empty input when we are running that workflow

[tlawrie]

Hi @AndreiOla @BenjaminRuby both of those conversations make sense.

I propose we go with an additional element instead of value being returned in the payload to the UI (but not part of the model so it is never sent to the server). This ensures that no value (encrypted or not) is sent for secure when not needed, as @AndreiOla rightly mentioned.

This is an approach used in other IBM products for secure type parameters.

In essence, if hidden=true then it says the value is there but hidden and this can be used for the additional logic. If hidden=false then there is no value on the server to encrypt.

We should still show to the user ******* or similar as needed.

parameters: [
    {
        "required": true,
        "placeholder": null,
        "language": null,
        "disabled": null,
        "defaultValue": "a",
        "value": null,
        "readOnly": false,
        "jsonPath": "",
        "description": "enter a token please",
        "key": "token",
        "label": "Token",
        "type": "text",
        "helperText": null
    },
    {
        "required": false,
        "placeholder": null,
        "language": null,
        "disabled": null,
        "readOnly": false,
        "jsonPath": "",
        "description": "This is a secure value.",
        "key": "password",
        "label": "Password",
        "type": "secure",
        "helperText": null,
        **"hidden": true**
    }
]

[AndreiOla]

@tlawrie @BenjaminRuby If we want to go this route, I would use some field that is already defined in AbstractConfigurationProperty. For example set the hidden field in options List"<"KeyValuePair">"?

[BenjaminRuby]

That approach works for me @tlawrie

@AndreiOla I will push up a version to IBM dev sometime today with the assumption that property ill have a hiddenValue field that lets me know there is value saved in the database that is not being shown to the user (I can show that with ****).

• passing hidden:true to an input causes our form to hide that input

[BenjaminRuby]

@tlawrie @AndreiOla one final thought - is it okay to run with an empty string and access the defaultValue in the DB? The secure/password input is a textInput and by default will set the value as an empty string. I can manipulate the values being passed to the service after the user hits Run if you would prefer me to update that to be null

[tlawrie]

Hi @AndreiOla, the OptionsList shouldn’t be used for this. It’s to provide options as part of a select. We will need to extend the model, but not the DB model, only the response going back to the browser.

RE the running with the defaultValue, I think we may want to have you pass null as a transformation, because empty string is again, a value of sorts… although would they ever be able to change the default value and make it empty?

On Wed, May 18, 2022 at 8:05 AM Benjamin Ruby @.***> wrote:

@tlawrie https://github.com/tlawrie @AndreiOla https://github.com/AndreiOla one final thought - is it okay to run with an empty string and access the defaultValue in the DB? The secure/password input is a textInput and by default will set the value as an empty string. I can manipulate the values being passed to the service after the user hits Run if you would prefer me to update that to be null

— Reply to this email directly, view it on GitHub https://github.com/boomerang-io/roadmap/issues/336#issuecomment-1129361785, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD5NMGY5BUG3DLBYK6BU6L3VKQJZXANCNFSM5SWQXCYQ . You are receiving this because you were mentioned.Message ID: @.***>

[BenjaminRuby]

Hi @AndreiOla, the OptionsList shouldn’t be used for this. It’s to provide options as part of a select. We will need to extend the model, but not the DB model, only the response going back to the browser. RE the running with the defaultValue, I think we may want to have you pass null as a transformation, because empty string is again, a value of sorts… although would they ever be able to change the default value and make it empty?

@tlawrie we currently wouldn't accept an empty string if the input is required - so I think it should be fine for me to just overwrite that as null for being passed back to the service when running the workflow

[AndreiOla]

I added a hiddenValue field in AbstractConfigurationProperty, and only set it when there is any filtered property. Please check the PR.