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

Support of binary attachments #115

Closed
mshedsilegx opened this issue Sep 21, 2023 · 10 comments · Fixed by #116
Closed

Support of binary attachments #115

mshedsilegx opened this issue Sep 21, 2023 · 10 comments · Fixed by #116
Assignees

Comments

@mshedsilegx
Copy link

Is it reasonable to consider the possibility to attach binary files to a secret ?
We do have a test case where we need to send PDF files as secrets. I thought it would be interesting to raise that topic.
I did a POC where I did a base64 encoding of a PDF as a secret, and it worked just fine, but I feel a binary attachment would provide a better user experience.

@Luzifer
Copy link
Owner

Luzifer commented Sep 23, 2023

Hmm in theory to have this working…

  • We would need to somehow store the file along the secret in an encrypted way
  • Store the filename so when the user downloads the file they would get myfile.pdf instead of download.txt
  • Prepare for someone to attach kinda big files (hope nobody gets to the idea to attach a 700Mi PSD)

So to achieve this we would need a new storage format… 🤔

  • We should still support plain secrets: I'm a very secret secret
  • We could add a new spec:
    OTSMeta{"v":1.0, "secret": "I'm ...", "attachment": {"name": "myfile.pdf", "data": "[b64enc data]"}}
    (maybe with some more special start sequence to ensure it doesn't accidentally collide with user secrets)

That way we could differentiate between the simple "I'm sharing a secret" and the advanced "I'm sharing structured data" usecase.

Reading the file from a HTML File-Select could be done through the FileReader API which gives us an ArrayBuffer (not yet sure how to serialize that into JSON, would need to fiddle with that but in some way that should be possible so Go as well as JS can read that)

  • Advantage of the new spec would be that we can extend that for further use-cases…
  • With the custom header in the spec a client tooling could be built to aid usage on CLI… (Would love that either way…)

There needs to be some limit of how big the attached files can get in order not to break the storage. Enforcement can be done through the transparent proxy (nginx, …) in front of OTS but the frontend should be able to determine the upload blob size and tell the user "nah mate, don't do that!"…

So…

  • …yeah, sounds like something we could do without adding much complexity (one file-selector for a single file - if you need multiple files use a ZIP - and a download-button) to the user-side.
  • Reading the file as a byte stream and passing it back should not provide any additional attack vector on the server but could of course be used to smuggle malicious content to the recipient (but that's nothing new and can be done through any file sharing so: 🤷🏻‍♀️)…
  • The only thing I could imagine is slower computers could have a hard time to decrypt a bigger payload. On the other hand, are such weak machines still out there? Also many machines do have hardware support for AES operations…
  • From what I read during my research for the code below there are limitations in size at ~50M so maybe other methods than demonstrated below are required… …or we just set the default file size limit to, dunno 20Mi or something which should be sufficient for most cases and for cases larger than that file-sharing services should be used. (Which also should solve the slow-machines issue mentioned above)

Would need to think about whether the frontend uses the simple format for simple text secrets or always uses the meta format which would always require CLI tooling when fetching from CLI and stop using the simple shell script included in the repo.

TL;DR: Sounds like something we could and can implement and I might have a plan… 😅

Code for a test-case to read file through `input`, serialize it and read back for download
<html>
  <style>
    #repr {
      font-family: monospace;
      white-space: pre-wrap;
    }
  </style>

  <input type="file" id="file">
  <div id="repr"></div>
  <a href="" download id="lnk">Download</a>

  <script>
    (() => {
      const file = document.querySelector('#file')
      const link = document.querySelector('#link')
      const repr = document.querySelector('#repr')

      /**
       * @param {ArrayBuffer} data
       * @returns string
       */
      function dataToBase64(data) {
        return btoa(String.fromCodePoint(...new Uint8Array(data)))
      }

      /**
       * @param {string} encoded
       * @returns ArrayBuffer
       */
      function base64ToData(encoded) {
        const binary = atob(encoded)
        return Uint8Array.from(binary, c => c.codePointAt(0)).buffer
      }

      function updateRepr() {
        if (file.files.length < 1) {
          repr.innerText = 'WTF? No file.'
          return
        }

        const f = file.files[0]
        f.arrayBuffer()
          .then(ab => {
            const attachmentContent = dataToBase64(ab)

            repr.innerText = `
              ${ab.byteLength}
              ${JSON.stringify({ attachment: { content: attachmentContent } })}
            `

            link.setAttribute('href', window.URL.createObjectURL(new Blob([base64ToData(attachmentContent)], { type: 'application/octet-stream' })))
          })
      }

      file.addEventListener('change', updateRepr)
    })()
  </script>
</html>

@Luzifer Luzifer self-assigned this Sep 23, 2023
@mshedsilegx
Copy link
Author

mshedsilegx commented Sep 25, 2023

Totally agree with everything you said, and thank you for the very detailed response. I would expect we would need:

  • a property to control the max size of attachment. Files for binary secrets are quite small in general (DER, PKCS8, PKCS12, JKS, PDF, etc).
  • another property to control the list of allowed extensions (ie deny all except - or allow all except)

I think the addition of an attachment feature is unique as far as OTS to my knowledge. It would be quite useful IMO. Thank you for considering this.

@Luzifer
Copy link
Owner

Luzifer commented Sep 25, 2023

another property to control the list of allowed extensions (ie deny all except - or allow all except)

Hmm not sure that really makes sense: There is no backend to check what has been uploaded as the backend only receives encrypted data. The only way to "limit" this would be to use the accept attribute of the file select (which only supports an allow-list, not a deny-list) and can easily be circumvented by just right-clicking, "inspect" and changing the attribute…

I mean yeah, adding that as a string is no pain so should do but that doesn't count as a security feature… 😄

@mshedsilegx
Copy link
Author

I understand, I still think it is useful as a first line of defense (allow list is even better for us). We are restricting the create secret functionality to trusted users via auth anyways.

@Luzifer Luzifer linked a pull request Oct 1, 2023 that will close this issue
@Luzifer
Copy link
Owner

Luzifer commented Oct 1, 2023

I've built the feature in #116 - some screenshots are included. What do you think?

@Luzifer
Copy link
Owner

Luzifer commented Oct 2, 2023

If you want to test the new feature: It's now active on ots.fyi

@mshedsilegx
Copy link
Author

Works very well.
1- A suggestion: should we display the list of files selected (with date and size) below the browse "n files selected" instead of in an over popup ?
2- I understand your point of a denied extension list, but can we implement as a first line of defense - ie deny all except an allow list (optional property). If for example, a user selects a .exe, it would generate an error and not upload.

@Luzifer
Copy link
Owner

Luzifer commented Oct 2, 2023

1- A suggestion: should we display the list of files selected (with date and size) below the browse "n files selected" instead of in an over popup ?

I don't get the point here… What popup are you talking about? Clicking the field opens a system file selector (we can't influence in any form)… 🤔

2- I understand your point of a denied extension list, but can we implement as a first line of defense - ie deny all except an allow list (optional property). If for example, a user selects a .exe, it would generate an error and not upload.

The configuration of the allow parameter can be done within the customize.yaml file. In the public instance that list currently is empty (so everything is allowed). You could for example configure

# Define which file types are selectable by the user when uploading
# files to attach. This fuels the `accept` attribute of the file
# select and requires the same format. Pay attention this is not
# suited as a security measure as this is purely a frontend
# implementation and can be circumvented.
# https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file#accept
acceptedFileTypes: 'image/*,text/*,application/pdf'

in order to allow images, text- and PDF-files. (Agreed, configuring this in the public instance probably makes sense to forbid users from posting binaries…)

@Luzifer
Copy link
Owner

Luzifer commented Oct 2, 2023

Ah saw the screenshot in the PR… …that's indeed system behavior, nothing I built…

@mshedsilegx
Copy link
Author

mshedsilegx commented Oct 2, 2023

ok, thanks for confirming (1).
(2) looks good. I like the MIME type approach.

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

Successfully merging a pull request may close this issue.

2 participants