Add speedscope remote viewer #33

gmcgibbon · 2021-07-20T18:04:42Z

Adds remote speedscope viewer for profiling on remote hosts. It uses rack middleware to hijack /app_profiler requests to list and view profiles of a booted rack / rails app. Very similar to what rack mini profiler does.

The routes are:

/app_profiler to list profiles
/app_profiler/{id} to show a specific profile in speedscope
/app_profiler/viewer/index.html to mount speedscope via Rack::File

dalehamel

Code itself looks fine, but I feel like this gem overall is starting to do too much, as it is now trying to cover both production and development profiling use-cases.

It would be nice if there was a safety to prevent trying to use this remove viewer if the app is in production, right now it relies on users installing the middleware for development only.

I'd like to also ask @Shopify/appsec review this since rails application security isn't my forte.

yaworsk · 2021-07-27T15:23:05Z

lib/app_profiler/viewer/speedscope_remote_viewer/base_middleware.rb

+                    <title>App Profiler</title>
+                  </head>
+                  <body>
+                    #{html}


I believe rendering like this leaves you vulnerable to XSS if html ever had anything malicious in it since ActiveSupport provides the default sanitizations

sanitize belongs to Action View which IIRC uses https://github.com/rails/rails-html-sanitizer. I think I can use that here.

yaworsk · 2021-07-27T15:25:20Z

lib/app_profiler/viewer/speedscope_remote_viewer/middleware.rb

+                iframe.style.width = '100vw';
+                iframe.style.height = '100vh';
+                iframe.style.border = 'none';
+                iframe.setAttribute('src', '/app_profiler/viewer/index.html#profileURL=' + objUrl + '&title=' + 'Flamegraph for #{name}');


similarly here - not sure if name can / will ever be user controlled, but this looks like it would lead to XSS

The name would be the match of \A/app_profiler/(.*)\z in the path as long as it maps to a file's basename without extension (eg. tmp/app_profiler/my_profile-4.json => my_profile-4) . Since the filename is sanitized I don't think we need to worry, but I can remove the interpolation and make the title generalized instead if you'd like.

Where does this sanitization/creation of this filename happen?

Here:

app_profiler/lib/app_profiler/profile.rb

Line 66 in 576c29a

def path

yaworsk · 2021-07-27T15:27:34Z

lib/app_profiler/yarn/command.rb

+      def yarn(command)
+        setup_yarn unless yarn_setup
+        exec("yarn #{command}") do
+          raise YarnError, "Failed to run #{command}."
+        end
+      end


This seems very dangerous. Would highly recommend we not just accept any command pass it to yarn. I don't know it well enough but if there's any flag that yarn takes that could result in arbitrary read/write, it would be bad. Would recommend instead we allow-list the commands or actual write the methods with the allowed yarn commands we'd accept

This was already in the codebase, I just moved it. Will address in a separate commit.

yaworsk · 2021-07-27T15:28:34Z

lib/app_profiler/yarn/command.rb

+      def exec(command)
+        system(command).tap do |return_code|
+          yield unless return_code
+        end


Similarly, this seems needlessly dangerous. Instead of accepting any command and passing it to system, we should allow list what we want to run or just write individual methods with no variability

It is a private method and the only consumer is yarn. I can inline it if that would make sense to you?

dalehamel

pending appsec feedback

NicholasEllul

Left a comment to help add more proactive security around the call to system(...). Pete mentioned, the most secure way to use the system(...) function is to have an allowlist of things that it can do. You do a great job of this with the ensure_command_valid? command, and now the only remaining place of ambiguity is in the options parameter.

From what I can see, yarn init only gets called with the --yes option, and yarn add only gets called with the speedscope option followed by either the file path, or --dev --ignore-workspace-root-check. Narrowing the scope of potential inputs and enforcing devs working on this in the future to be explicit about what they intend to allow will minimize the risk of something slipping between the cracks into this powerful method

NicholasEllul · 2021-09-15T18:36:31Z

lib/app_profiler/yarn/command.rb

+      def exec(*command)
+        system(*command).tap do |return_code|
+          yield unless return_code
+        end
+      end


Since we would never want to run exec without calling ensure_command_valid? can we move the call to that function here? That way down the road if exec gets reused somewhere, we can trust that it will always have its safeguards for checking the command bundled into it.

I changed it to permit all command strings. This should be the most secure option despite the cost of iteration and matching. I can instead hardcode these strings into the methods and forgo allow-listing, but I don't know how you feel about that.

hardcode these strings into the methods and forgo allow-listing

Can you expand on what you mean by this? What you've done here is fantastic with the allow list, if we can maintain a similar strict list of commands another way that you feel is more efficient i'm ok with that as well 👍

Sorry, I meant multiple allow-lists. I was originally permitting specific subcommands to yarn, and then adding another list for exec, but permitting commands + options once in exec simplified things and sidestepped allowing invalid things like yarn init speedscope --dev.

Co-authored-by: Pragya Daga <pragya.daga@shopify.com>

NicholasEllul

Left a comment, but i'm happy with the allow list that was added! Re-request a review if theres any changes to that you need me to look over

gmcgibbon force-pushed the view_middleware branch 2 times, most recently from 54b47dd to 19ad44a Compare July 20, 2021 18:09

gmcgibbon requested a review from a team July 26, 2021 21:20

dalehamel approved these changes Jul 27, 2021

View reviewed changes

yaworsk reviewed Jul 27, 2021

View reviewed changes

dalehamel requested changes Jul 28, 2021

View reviewed changes

gmcgibbon force-pushed the view_middleware branch 3 times, most recently from eb43259 to a70273d Compare August 16, 2021 21:36

gmcgibbon requested review from yaworsk and dalehamel August 16, 2021 21:37

gmcgibbon removed the request for review from yaworsk September 14, 2021 21:02

NicholasEllul reviewed Sep 15, 2021

View reviewed changes

gmcgibbon force-pushed the view_middleware branch 2 times, most recently from ae4efd6 to fd5cd2e Compare September 15, 2021 22:24

gmcgibbon and others added 2 commits September 15, 2021 18:25

Add speedscope remote viewer

4851178

Co-authored-by: Pragya Daga <pragya.daga@shopify.com>

Permit commands

8e7a93d

gmcgibbon force-pushed the view_middleware branch from fd5cd2e to 8e7a93d Compare September 15, 2021 22:29

gmcgibbon requested a review from NicholasEllul September 16, 2021 15:29

NicholasEllul approved these changes Sep 16, 2021

View reviewed changes

gmcgibbon merged commit f8746b0 into main Sep 16, 2021

gmcgibbon deleted the view_middleware branch September 16, 2021 19:28

shopify-shipit bot temporarily deployed to production March 2, 2022 15:30 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add speedscope remote viewer #33

Add speedscope remote viewer #33

gmcgibbon commented Jul 20, 2021 •

edited

Loading

dalehamel left a comment

yaworsk Jul 27, 2021

gmcgibbon Aug 16, 2021

yaworsk Jul 27, 2021

gmcgibbon Aug 16, 2021

NicholasEllul Sep 15, 2021

gmcgibbon Sep 15, 2021

NicholasEllul Sep 15, 2021

yaworsk Jul 27, 2021

gmcgibbon Aug 16, 2021

yaworsk Jul 27, 2021

gmcgibbon Aug 16, 2021

dalehamel left a comment

NicholasEllul left a comment

NicholasEllul Sep 15, 2021

gmcgibbon Sep 15, 2021 •

edited

Loading

NicholasEllul Sep 16, 2021 •

edited

Loading

gmcgibbon Sep 16, 2021 •

edited

Loading

NicholasEllul left a comment

Add speedscope remote viewer #33

Add speedscope remote viewer #33

Conversation

gmcgibbon commented Jul 20, 2021 • edited Loading

dalehamel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalehamel left a comment

Choose a reason for hiding this comment

NicholasEllul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcgibbon Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

NicholasEllul Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

gmcgibbon Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

NicholasEllul left a comment

Choose a reason for hiding this comment

gmcgibbon commented Jul 20, 2021 •

edited

Loading

gmcgibbon Sep 15, 2021 •

edited

Loading

NicholasEllul Sep 16, 2021 •

edited

Loading

gmcgibbon Sep 16, 2021 •

edited

Loading