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

Add a Content Security Policy to non-rustdoc pages #1333

Merged
merged 3 commits into from
Apr 8, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ font-awesome-as-a-crate = { path = "crates/font-awesome-as-a-crate" }
dashmap = "3.11.10"
string_cache = "0.8.0"
postgres-types = { version = "0.2", features = ["derive"] }
getrandom = "0.2.1"

# Async
tokio = { version = "1.0", features = ["rt-multi-thread"] }
Expand Down
5 changes: 5 additions & 0 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ pub struct Config {
// For unit-tests the number has to be higher.
pub(crate) random_crate_search_view_size: u32,

// Content Security Policy
pub(crate) csp_report_only: bool,

// Build params
pub(crate) build_attempts: u16,
pub(crate) rustwide_workspace: PathBuf,
Expand Down Expand Up @@ -96,6 +99,8 @@ impl Config {

random_crate_search_view_size: env("DOCSRS_RANDOM_CRATE_SEARCH_VIEW_SIZE", 500)?,

csp_report_only: env("DOCSRS_CSP_REPORT_ONLY", false)?,

rustwide_workspace: env("CRATESFYI_RUSTWIDE_WORKSPACE", PathBuf::from(".workspace"))?,
inside_docker: env("DOCS_RS_DOCKER", false)?,
local_docker_image: maybe_env("DOCS_RS_LOCAL_DOCKER_IMAGE")?,
Expand Down
192 changes: 192 additions & 0 deletions src/web/csp.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,192 @@
use crate::config::Config;
use iron::{AfterMiddleware, BeforeMiddleware, IronResult, Request, Response};

pub(super) struct Csp {
nonce: String,
suppress: bool,
}

impl Csp {
fn new() -> Self {
// Nonces need to be different for each single request in order to maintain security, so we
// generate a new one with a cryptographically-secure generator for each request.
let mut random = [0u8; 36];
getrandom::getrandom(&mut random).expect("failed to generate a nonce");
jyn514 marked this conversation as resolved.
Show resolved Hide resolved

Self {
nonce: base64::encode(&random),
suppress: false,
}
}

pub(super) fn suppress(&mut self, suppress: bool) {
self.suppress = suppress;
}
Comment on lines +22 to +24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make the suppress field private if you're going to allow setting it anyway?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly out of habit, and because this looks weird:

     req.extensions
        .get_mut::<Csp>()
        .expect("missing CSP")
        .suppress = true; 


pub(super) fn nonce(&self) -> &str {
&self.nonce
}

fn render(&self, content_type: ContentType) -> Option<String> {
if self.suppress {
return None;
}
let mut result = String::new();

// Disable everything by default
result.push_str("default-src 'none'");

// Disable the <base> HTML tag to prevent injected HTML content from changing the base URL
// of all relative links included in the website.
result.push_str("; base-uri 'none'");

// Allow loading images from the same origin. This is added to every response regardless of
// the MIME type to allow loading favicons.
//
// Images from other HTTPS origins are also temporary allowed until issue #66 is fixed.
result.push_str("; img-src 'self' https:");

match content_type {
ContentType::Html => self.render_html(&mut result),
ContentType::Svg => self.render_svg(&mut result),
ContentType::Other => {}
}

Some(result)
}

fn render_html(&self, result: &mut String) {
// Allow loading any CSS file from the current origin.
result.push_str("; style-src 'self'");

// Allow loading any font from the current origin.
result.push_str("; font-src 'self'");

// Only allow scripts with the random nonce attached to them.
//
// We can't just allow 'self' here, as users can upload arbitrary .js files as part of
// their documentation and 'self' would allow their execution. Instead, every allowed
// script must include the random nonce in it, which an attacker is not able to guess.
Comment on lines +68 to +69
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh, I just realized while reading this that there's a new nonce for each request, it's not a hash of the file or anything like that. Maybe add a comment to the CSP saying that? And a test that two sequential requests get different nonces?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the test is already there. I'll add a comment.

result.push_str(&format!("; script-src 'nonce-{}'", self.nonce));
}

fn render_svg(&self, result: &mut String) {
// SVG images are subject to the Content Security Policy, and without a directive allowing
// style="" inside the file the image will be rendered badly.
result.push_str("; style-src 'self' 'unsafe-inline'");
}
}

impl iron::typemap::Key for Csp {
type Value = Csp;
}

enum ContentType {
Html,
Svg,
Other,
}

pub(super) struct CspMiddleware;

impl BeforeMiddleware for CspMiddleware {
fn before(&self, req: &mut Request) -> IronResult<()> {
req.extensions.insert::<Csp>(Csp::new());
Ok(())
}
}

impl AfterMiddleware for CspMiddleware {
fn after(&self, req: &mut Request, mut res: Response) -> IronResult<Response> {
let config = req
.extensions
.get::<Config>()
.expect("missing Config")
.clone();
let csp = req.extensions.get_mut::<Csp>().expect("missing CSP");

let content_type = res
.headers
.get_raw("Content-Type")
.and_then(|headers| headers.get(0))
.map(|header| header.as_slice());

let preset = match content_type {
Some(b"text/html; charset=utf-8") => ContentType::Html,
Some(b"text/svg+xml") => ContentType::Svg,
_ => ContentType::Other,
};

if let Some(rendered) = csp.render(preset) {
res.headers.set_raw(
// The Report-Only header tells the browser to just log CSP failures instead of
// actually enforcing them. This is useful to check if the CSP works without
// impacting production traffic.
if config.csp_report_only {
"Content-Security-Policy-Report-Only"
} else {
"Content-Security-Policy"
},
vec![rendered.as_bytes().to_vec()],
);
}
Ok(res)
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_random_nonce() {
let csp1 = Csp::new();
let csp2 = Csp::new();
assert_ne!(csp1.nonce(), csp2.nonce());
}

#[test]
fn test_csp_suppressed() {
let mut csp = Csp::new();
csp.suppress(true);

assert!(csp.render(ContentType::Other).is_none());
assert!(csp.render(ContentType::Html).is_none());
assert!(csp.render(ContentType::Svg).is_none());
}

#[test]
fn test_csp_other() {
let csp = Csp::new();
assert_eq!(
Some("default-src 'none'; base-uri 'none'; img-src 'self' https:".into()),
csp.render(ContentType::Other)
);
}

#[test]
fn test_csp_svg() {
let csp = Csp::new();
assert_eq!(
Some(
"default-src 'none'; base-uri 'none'; img-src 'self' https:; \
style-src 'self' 'unsafe-inline'"
.into()
),
csp.render(ContentType::Svg)
);
}

#[test]
fn test_csp_html() {
let csp = Csp::new();
assert_eq!(
Some(format!(
"default-src 'none'; base-uri 'none'; img-src 'self' https:; \
style-src 'self'; font-src 'self'; script-src 'nonce-{}'",
csp.nonce()
)),
csp.render(ContentType::Html)
);
}
}
5 changes: 5 additions & 0 deletions src/web/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ macro_rules! extension {
mod build_details;
mod builds;
mod crate_details;
mod csp;
mod error;
mod extensions;
mod features;
Expand All @@ -94,6 +95,7 @@ mod statics;

use crate::{impl_webpage, Context};
use chrono::{DateTime, Utc};
use csp::CspMiddleware;
use error::Nope;
use extensions::InjectExtensions;
use failure::Error;
Expand Down Expand Up @@ -128,6 +130,9 @@ impl CratesfyiHandler {
let mut chain = Chain::new(base);
chain.link_before(inject_extensions);

chain.link_before(CspMiddleware);
chain.link_after(CspMiddleware);

chain
}

Expand Down
20 changes: 19 additions & 1 deletion src/web/page/web_page.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use super::TemplateData;
use crate::ctry;
use crate::web::csp::Csp;
use iron::{headers::ContentType, response::Response, status::Status, IronResult, Request};
use serde::Serialize;
use std::borrow::Cow;
Expand Down Expand Up @@ -35,12 +36,29 @@ macro_rules! impl_webpage {
};
}

#[derive(Serialize)]
struct TemplateContext<'a, T> {
csp_nonce: &'a str,
#[serde(flatten)]
page: &'a T,
}

/// The central trait that rendering pages revolves around, it handles selecting and rendering the template
pub trait WebPage: Serialize + Sized {
/// Turn the current instance into a `Response`, ready to be served
// TODO: We could cache similar pages using the `&Context`
fn into_response(self, req: &Request) -> IronResult<Response> {
let ctx = Context::from_serialize(&self).unwrap();
let csp_nonce = req
.extensions
.get::<Csp>()
.expect("missing CSP from the request extensions")
.nonce();

let ctx = Context::from_serialize(&TemplateContext {
csp_nonce,
page: &self,
})
.unwrap();
let rendered = ctry!(
req,
req.extensions
Expand Down
8 changes: 7 additions & 1 deletion src/web/rustdoc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
db::Pool,
utils,
web::{
crate_details::CrateDetails, error::Nope, file::File, match_version,
crate_details::CrateDetails, csp::Csp, error::Nope, file::File, match_version,
metrics::RenderingTimesRecorder, redirect_base, MatchSemver, MetaData,
},
Config, Metrics, Storage,
Expand Down Expand Up @@ -253,6 +253,12 @@ pub fn rustdoc_html_server_handler(req: &mut Request) -> IronResult<Response> {
let metrics = extension!(req, Metrics).clone();
let mut rendering_time = RenderingTimesRecorder::new(&metrics.rustdoc_rendering_times);

// Pages generated by Rustdoc are not ready to be served with a CSP yet.
req.extensions
.get_mut::<Csp>()
.expect("missing CSP")
.suppress(true);
Comment on lines +257 to +260
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shared between all threads, isn't it? Won't this surpress the CSP policy for all requests? Or is it per-request because you used link_before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, all the extensions are added in each request. Most of the others are just cloned Arc<_>s, instead here it's a fresh instance every time.


// Get the request parameters
let router = extension!(req, Router);

Expand Down
11 changes: 5 additions & 6 deletions templates/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

<title>{%- block title -%} Docs.rs {%- endblock title -%}</title>

<script type="text/javascript">{%- include "theme.js" -%}</script>
<script nonce="{{ csp_nonce }}">{%- include "theme.js" -%}</script>
{%- block css -%}{%- endblock css -%}
</head>

Expand All @@ -29,11 +29,10 @@
{%- block header %}{% endblock header -%}

{%- block body -%}{%- endblock body -%}
</body>

<script type="text/javascript" src="/-/static/menu.js?{{ docsrs_version() | slugify }}"></script>
<script type="text/javascript" src="/-/static/index.js?{{ docsrs_version() | slugify }}"></script>

{%- block javascript -%}{%- endblock javascript -%}
<script type="text/javascript" nonce="{{ csp_nonce }}" src="/-/static/menu.js?{{ docsrs_version() | slugify }}"></script>
<script type="text/javascript" nonce="{{ csp_nonce }}" src="/-/static/index.js?{{ docsrs_version() | slugify }}"></script>

{%- block javascript -%}{%- endblock javascript -%}
</body>
</html>
2 changes: 1 addition & 1 deletion templates/core/home.html
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ <h1 class="brand">{{ "cubes" | fas(fw=true) }} Docs.rs</h1>
{%- endblock body -%}

{%- block javascript -%}
<script type="text/javascript" charset="utf-8">
<script type="text/javascript" nonce="{{ csp_nonce }}">
function getKey(ev) {
if ("key" in ev && typeof ev.key != "undefined") {
return ev.key;
Expand Down
4 changes: 2 additions & 2 deletions templates/crate/details.html
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@
{%- if details.documented_items and details.total_items -%}
{% set percent = details.documented_items * 100 / details.total_items %}
<li class="pure-menu-heading">Coverage</li>
<li class="pure-menu-item" style="text-align:center;"><b>{{ percent | round(precision=2) }}%</b><br>
<li class="pure-menu-item text-center"><b>{{ percent | round(precision=2) }}%</b><br>
<span class="documented-info"><b>{{ details.documented_items }}</b> out of <b>{{ details.total_items }}</b> items documented</span>
{%- if details.total_items_needing_examples and details.items_with_examples -%}
<span style="font-size: 13px;"><b>{{ details.items_with_examples }}</b> out of <b>{{ details.total_items_needing_examples }}</b> items with examples</span>
<span class="documented-info"><b>{{ details.items_with_examples }}</b> out of <b>{{ details.total_items_needing_examples }}</b> items with examples</span>
{%- endif -%}
</li>
{%- endif -%}
Expand Down
6 changes: 3 additions & 3 deletions templates/crate/features.html
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,18 @@
{%- if features -%}
{%- for feature in features -%}
<li class="pure-menu-item">
<a href="#{{ feature.name }}" class="pure-menu-link" style="text-align:center;">
<a href="#{{ feature.name }}" class="pure-menu-link text-center">
{{ feature.name }}
</a>
</li>
{%- endfor -%}
{%- elif features is iterable -%}
<li class="pure-menu-item">
<span style="font-size: 13px;">This release does not have any feature flags.</span>
<span class="documented-info">This release does not have any feature flags.</span>
</li>
{%- else -%}
<li class="pure-menu-item">
<span style="font-size: 13px;">Feature flags data are not available for this release.</span>
<span class="documented-info">Feature flags data are not available for this release.</span>
</li>
{%- endif -%}
</ul>
Expand Down
Loading