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

Disallow calling "helperMissing" and "blockHelperMissing" directly #1558

Closed
nknapp opened this issue Sep 21, 2019 · 12 comments
Closed

Disallow calling "helperMissing" and "blockHelperMissing" directly #1558

nknapp opened this issue Sep 21, 2019 · 12 comments

Comments

@nknapp
Copy link
Collaborator

nknapp commented Sep 21, 2019

The recent remote-code-execution exploits where misusing the helper blockHelperMissing in order to call methods from object prototypes that actually should not have been called.

The helpers "helperMissing" and "blockHelperMissing" are not meant to be called directly as in

{{helperMissing}}
{{#helperMissing}}{{/helperMissing}}
{{blockHelperMissing arg}}
{{#blockHelperMissing arg}}{{/blockHelperMissing}}

We can offer a flag that allows execution in case someone really wants it, but the default will be not to allow this.

Note that although calling {{helperMissing}} from the template is not part of the documented API and I assume it makes no sense other then to build an exploit. From this point of view this is not a breaking change an will result in a minor version update only (because of the feature flag).

Even though it is not documented, I would rather try to maintain compatibility here, but since this a security-relevant change I will not do that in this case.

@nknapp nknapp closed this as completed in 2078c72 Sep 24, 2019
@nknapp nknapp mentioned this issue Sep 26, 2019
4 tasks
aszlig added a commit to aszlig/avonc that referenced this issue Dec 12, 2019
Okay, so since I'm pretty much busy with other work I'm really getting
fed up by all those apps *not* providing changelogs.

Instead of spending hours of searching the corresponding repositories,
diffing all the changes and summarise them just to provide an adequate
changelog (as I've done in the past, see Git history), I decided to ping
the maintainers to (hopefully) encourage them to write changelogs.

Apart from just my little project here, I think even users of the
updater UI deserve to know what has changed and what they're going to
give access to their private data.

So if you're one of the maintainers pinged here:

  Can you please, please, *PLEASE* provide a changelog when updating
  your apps on apps.nextcloud.com?

Here we go, these are the apps updated without changelogs:

  .--------------------+--------+---------+-------------------.
  | App                | From   | To      | Author/Maintainer |
  +--------------------+--------+---------+-------------------+
  | bookmarks          | 2.3.1  | 2.3.3   | @marcelklehr      |
  | carnet             | 0.18.7 | 0.18.12 | @PhieF            |
  | circles            | 0.17.9 | 0.17.10 | @daita            |
  | cms_pico           | 0.9.8  | 1.0.2   | @PhrozenByte      |
  | cookbook           | 0.5.4  | 0.5.5   | @mrzapp           |
  | dicomviewer        | 1.2.0  | 1.2.1   | @ayselafsar       |
  | discoursesso       | 0.9.16 | 0.9.17  | @soudis           |
  | files_antivirus    | 2.2.0  | 2.2.1   | @rullzer          |
  | files_photospheres | 1.0.5  | 1.0.6   | @R0Wi             |
  | files_readmemd     | 1.1.2  | 1.1.3   | @mamatt           |
  | flowupload         | 0.1.5  | 0.1.7   | @e-alfred         |
  | music              | 0.11.0 | 0.11.1  | @paulijar         |
  | ocr                | 4.4.16 | 4.4.25  | @janis91          |
  | oidc_login         | 1.0.3  | 1.1.0   | @pulsejet         |
  | passman            | 2.3.3  | 2.3.5   | @newhinton        |
  | sharingpath        | 0.0.3  | 0.1.0   | @rookie0          |
  | terms_of_service   | 1.3.0  | 1.3.1   | @nickvergessen    |
  | timetracker        | 0.0.34 | 0.0.39  | @puthre           |
  | user_cas           | 1.7.4  | 1.8.0   | @felixrupp        |
  | zimbradrive        | 0.8.23 | 0.8.24  | @bud-mo           |
  `--------------------+--------+---------+-------------------'

Hopefully if I start to annoy everybody frequently enough on every new
update, maybe more people will start providing changelogs.

However, if things don't improve I will either start writing a series of
scrapers to fetch the upstream changelogs (which I'm glad at least most
of the apps mentioned above have) or automate annoying maintainers :-D

So, let's continue with the apps that *actually* provide a changelog.

Apps updated for Nextcloud 15, 16 and 17:

  passwords (2019.11.1 -> 2019.12.0):

    * Add lazy loading for favicons and avatars
    * Fix server timeout on settings page
    * Fix mobile layout in NC 17
    * Fix navigation in NC 15
    * Attempt to send mails to users without email

    Kudos/thanks to @marius-wieschollek, you're awesome!

  previewgenerator (2.1.0 -> 2.2.0):

    * Add Nextcloud 18 support

    Kudos/thanks to @rullzer, even though not providing a changelog for
    files_antivirus.

  richdocuments (3.4.4 -> 3.4.6):

    Changes for version 3.4.6:

      * Force read operation to trigger audit log when issuing a token
      * Add Nextcloud 18 compatibility

    Changes for version 3.4.5:

      * Retry putContent operation if locked
      * Include locale in the loleaflet lang parameter
      * Make sure files created from the same template have a different
        WOPI file id
      * Always use the owner file owner to access for share links
      * Make sure Firefox doesn't navigate out of the current directory
      * Dependency bumps

   Kudos/thanks to @juliushaertl, you're awesome!

  secsignid (0.2.4 -> 0.2.5):

    * Fix issue regarding integrity check

    Kudos/thanks to @BjoernSecSign, you're awesome!

Apps updated for Nextcloud 16 and 17 only:

  news (14.0.1 -> 14.0.2):

    * Get content:encoded of item if available
    * Update js and php dependencies
    * Generate enclosure div only for audio & video

    Kudos/thanks to @Grotax, you're awesome!

  twofactor_gateway (0.14.1 -> 0.15.0):

    * Add Spryng gateway support
    * Add Sms77io gateway support
    * Add Nextcloud 18 support
    * Add php7.4 support
    * Update ecall API to new version
    * Add/update translations
    * Remove PHP 7.1 support

    Kudos/thanks to @ChristophWurst, you're awesome!

Apps updated for Nextcloud 17 only:

  mail (0.18.1 -> 0.20.1):

    Changes for version 0.20.1:

      * Add occ command to diagnose an account
      * Add/update translations
      * Update dependencies
      * Fix provisioned account update password check when password is
        set to empty string

    Changes for version 0.20.0:

      * Add admin settings UI to configure provisioned accounts
      * Default account configuration via config.php

    Changes for version 0.19.1:

      * Add/update translations
      * Update dependencies

    Changes for version 0.19.0:

      * Add PHP 7.4 support
      * Mail detail view now only has one back/menu button
      * Composer attachment buttons moved into action menu
      * Simpler wording for plain text/rich text switch
      * Fix opening messages with attachment content-type and
        content-disposition
      * Fix spacing in settings section
      * Fix missing popover for addresses without a display name
      * Fix horizontal scrolling issue on mobile
      * Show recipient avatar instead of sender avatar in \sent
        mailboxes
      * Focus "to" field automatically when writing a new message
      * Fix virtual flagged inbox shown as child of the inbox
      * Add missing label of unified inbox
      * Fix .ics attachment mime detection
      * Fix .ics attachment import
      * Fix unwanted flowed text formatting
      * Remove PHP 7.1 support

    Kudos/thanks to @ChristophWurst, you're awesome... again!

  phonetrack (0.5.10 -> 0.5.11):

    * Fix auto export cron job failing on some setups

    Kudos/thanks to @eneiluj, you're awesome!

  sentry (5.1.0 -> 6.0.1):

    Changes for version 6.0.1:

      * Add new Sentry SDKs
      * Fix application class instantiation warning

    Changes for version 6.0.0:

      * Add PHP 7.4 support
      * Add Nextcloud 18 support
      * Add new Sentry JavaScript SDK
      * Fix "app name as tag" regression
      * Remove PHP 7.1 support

    Changes for version 5.2.0:

      * Add new Sentry JavaScript SDK
      * Updated dependencies

    @ChristophWurst again! Thanks a lot!

  suspicious_login (2.4.1 -> 3.0.0):

    * Add PHP 7.4 support
    * Add/update translations
    * Update dependencies
    * Fix IPv6 optimizer
    * Remove PHP 7.1 support

    Did I mention "Kudos/thanks to @ChristophWurst"? ;-)

  twofactor_nextcloud_notification (2.1.1 -> 2.2.0):

    * Make Nextcloud 18 compatible

    This makes 1 app without changelog and 2 for @rullzer, so
    thanks/kudos to you! You're awesome!

  twofactor_totp (4.0.0 -> 4.1.0):

    * Add Nextcloud 18 support
    * Add PHP 7.4 support
    * Add/update translations
    * Update dependencies

    Well, @ChristophWurst at it again!

  twofactor_u2f (4.0.0 -> 5.0.0):

    * Add Nextcloud 18 support
    * Add PHP 7.4 support
    * Add/update translations
    * Update dependencies
    * Fix Npm dependency vulnerability:
      handlebars-lang/handlebars.js#1558
    * Fix unnecessarily big challenge bundle
    * Remove PHP 7.1 support

    Guess who? It's either about some sausage or their name just doesn't
    matter, who knows? Of course: @ChristophWurst

New apps added to Nextcloud 16 and 17:

  analytics - Data analytics/visualisation for Nextcloud and ownCloud

Also, the breeze-dark theme update for Nextcloud 17 contains the
following changes:

  * Add app-specific favicons
  * Add icon for the "circles" app
  * Add styling for "bookmarks" app
  * Fix hover styling for notifications
  * Add icons for disabled users
  * Add personal icon in "activity" app
  * Allow for custom app folder/style icon for individual U2F devices
  * Change login fields to be background-dark
  * Change icon used for delete-hover in "quicknotes"

Now looking up all these authors/maintainers took a little bit more than
an hour*, which I think won't scale very well in the future. Especially I
want to avoid annoying people who *do* provide a changelog, so I'm going
to just call out everybody who *doesn't* provide one in the future.

* -> An hour for digging up the maintainers - reviewing the diffs for
     the apps I actually use took way longer...

Signed-off-by: aszlig <aszlig@nix.build>
@NicoleG25
Copy link

NicoleG25 commented Jan 9, 2020

@nknapp Does 7b67a29 fix CVE-2019-19919 ? :)

@nknapp
Copy link
Collaborator Author

nknapp commented Jan 11, 2020

Please look at the docs. Under api-reference -> runtime options. I have added links to all npm advisories regarding prototype access that are public so far. They contain the resolved versions. I think your CVE has an advisory linked, so you can compare it.

I can't post any links right now, doesn't work with my mobile phone.

@nknapp
Copy link
Collaborator Author

nknapp commented Jan 11, 2020

You can also search advisories. Just add a "search=handlebars" query param when you are on the advisory list at npmjs.com

@rohitchoudhary87
Copy link

MongoDB Connected
Handlebars: Access has been denied to resolve the property "name" because it is not an "own property" of its parent.
You can add a runtime option to disable the check or this warning:
See https://handlebarsjs.com/api-reference/runtime-options.html#options-to-control-prototype-access for details

hey
I am getting this error. please help

@nknapp
Copy link
Collaborator Author

nknapp commented Jan 18, 2020

Are you using handlebars directly in your project, or is it used by another dependency of yours?

@rohitchoudhary87
Copy link

rohitchoudhary87 commented Jan 18, 2020 via email

@nknapp
Copy link
Collaborator Author

nknapp commented Jan 18, 2020

Have you had a look at the link to the documentation? It explains the thoughts behind the new limitation and the runtime-options that control it. If the documentation is not good enough, I would like to know what is missing. I can explain of here as well, but please have a look at the documentation before.

@rohitchoudhary87
Copy link

I have read the documentation but I unable to figure out the problem. earlier it was working just fine.
In this project, the fields are getting stored in the database.
but as soon as I try to fetch them out and show in the UI its showing this error.

//menu index
router.get('/index' , (req,res) => {
Menu.find({})
.then(menus => {
res.render('menu/index' , {
menus: menus,
});
});
});

in this block the error is generating when i output {{#each menus}} {{name}} {{/each}}

@nknapp
Copy link
Collaborator Author

nknapp commented Jan 18, 2020

The problem is that "name" is defined on the prototype of the object you are using as input. See #1635 for details about the change that forbids that.

You are not using handlebars directly. You are using "express-hbs" or something like that. I am not sure how to pass a runtime option to the template with that framework.

What handlebars-related dependencies do you have in your package.json exactly? I can look for a solution but there are multiple similar packages and I need to know which one you are using

@rohitchoudhary87
Copy link

I am using express handlebars.

"express-handlebars": "^3.1.0",

@nknapp
Copy link
Collaborator Author

nknapp commented Jan 18, 2020

I'd like to move this discussion to #1642, which I just created for this kind of question (I have some of them already)

@rohitchoudhary87
Copy link

Thank You @nknapp . I figured out the problem.

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

3 participants