diff --git a/CHANGELOG.md b/CHANGELOG.md index 251168b..cb94ae0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,10 +6,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] ### Added +- Security regression tests for XSS prevention in node name, version, diff, stats views and HTMLEntities encoding of device config content (@mattimustang, @robertcheramy) ### Changed +- Remove the no longer needed escape_once helper call in node.haml, relying on HAML's global escape_html instead (@robertcheramy) ### Fixed +- Fix XSS vulnerability (CWE-79) by enabling HAML's escape_html globally; user-controlled values in node names, group names, model names, and URL parameters are now HTML-escaped in all templates (@mattimustang) ## [0.18.1 – 2026-01-19] diff --git a/lib/oxidized/web/views/layout.haml b/lib/oxidized/web/views/layout.haml index 5a8ee65..4c364b8 100644 --- a/lib/oxidized/web/views/layout.haml +++ b/lib/oxidized/web/views/layout.haml @@ -33,6 +33,6 @@ %i.bi.bi-moon-fill .container-fluid - =yield + != yield !=haml :footer diff --git a/lib/oxidized/web/views/node.haml b/lib/oxidized/web/views/node.haml index 788fa1e..ec3f252 100644 --- a/lib/oxidized/web/views/node.haml +++ b/lib/oxidized/web/views/node.haml @@ -16,6 +16,6 @@ %i.bi.bi-repeat %pre.bg-body-tertiary.border.border-secondary-subtle.rounded %code - =escape_once(JSON.pretty_generate(@data)) + =JSON.pretty_generate(@data) diff --git a/lib/oxidized/web/webapp.rb b/lib/oxidized/web/webapp.rb index 5e69e3d..f58823c 100644 --- a/lib/oxidized/web/webapp.rb +++ b/lib/oxidized/web/webapp.rb @@ -11,7 +11,7 @@ module API class WebApp < Sinatra::Base helpers Sinatra::UrlForHelper set :public_folder, proc { File.join(root, 'public') } - set :haml, { escape_html: false } + set :haml, { escape_html: true } get '/' do redirect url_for('/nodes') diff --git a/spec/web/node/show_spec.rb b/spec/web/node/show_spec.rb index 725513c..98f44c3 100644 --- a/spec/web/node/show_spec.rb +++ b/spec/web/node/show_spec.rb @@ -125,5 +125,19 @@ def expected_json_body(hide_vars: false) _(@serialized_node[:vars]["enable"]).must_equal "secret_enable" end end + + describe "HTML escaping" do + it "escapes HTML special characters in var values" do + app.set(:configuration, { hide_node_vars: [] }) + @serialized_node[:vars][:enable] = "" + + get '/node/show/sw5' + _(last_response.ok?).must_equal true + body = last_response.body + + _(body).must_include("<script>alert(1)</script>") + _(body).wont_include("") + end + end end end diff --git a/spec/web/xss_spec.rb b/spec/web/xss_spec.rb new file mode 100644 index 0000000..64be699 --- /dev/null +++ b/spec/web/xss_spec.rb @@ -0,0 +1,126 @@ +require_relative '../spec_helper' +require 'cgi' + +describe Oxidized::API::WebApp do + include Rack::Test::Methods + + def app + Oxidized::API::WebApp + end + + before do + @nodes = mock('Oxidized::Nodes') + app.set(:nodes, @nodes) + end + + describe 'reflected XSS prevention' do + # Attack scenarios basing on the node name are highly hypothetical. The node + # name is used to look up the node in Oxidized before the view is rendered, + # so a non-existing node raises NodeNotFound and the payload is never + # displayed. + # Exploiting this would require the ability to manipulate the node names in + # Oxidized itself (i.e. control over the node source/backend). These tests + # mock the data layer to reach the rendering path and verify escaping. + it 'escapes node name in /node/version versions list' do + # Use a payload without '/' so the routing does not split on rpartition('/') + malicious = '' + @nodes.expects(:version).with(malicious, nil).returns([]) + + get "/node/version?node_full=#{CGI.escape(malicious)}" + + _(last_response.ok?).must_equal true + _(last_response.body).must_include('<img src=x onerror=alert(1)>') + _(last_response.body).wont_include('') + end + + it 'escapes node name in /node/version/view' do + malicious = '' + @nodes.expects(:get_version).with(malicious, '', 'abc123').returns('device config') + + get "/node/version/view?node=#{CGI.escape(malicious)}&group=&oid=abc123&epoch=0&num=1" + + _(last_response.ok?).must_equal true + _(last_response.body).must_include('<script>alert(1)</script>') + _(last_response.body).wont_include('') + end + + it 'escapes node name in /node/version/diffs' do + malicious = '' + versions = [{ oid: 'C006', time: Time.parse('2025-02-05 19:49:00 +0100') }] + @nodes.expects(:version).with(malicious, nil).returns(versions) + @nodes.expects(:get_diff).with(malicious, '', 'C006', nil).returns( + { patch: "- old line\n+ new line\n", stat: [1, 1] } + ) + + get "/node/version/diffs?node=#{CGI.escape(malicious)}&group=&oid=C006&epoch=0&num=1" + + _(last_response.ok?).must_equal true + _(last_response.body).must_include('<script>alert(1)</script>') + _(last_response.body).wont_include('') + end + end + + describe 'stored XSS prevention' do + it 'escapes node names in /nodes list' do + malicious = "'>" + @nodes.expects(:list).returns( + [{ name: malicious, ip: '10.0.0.1', model: 'ios', + full_name: malicious, group: 'default', time: Time.now, mtime: Time.now }] + ) + + get '/nodes' + + _(last_response.ok?).must_equal true + _(last_response.body).must_include(''><script>alert(1)</script>') + _(last_response.body).wont_include("'>") + _(last_response.body).wont_include('') + end + + it 'escapes node names in /nodes/stats' do + malicious = "'>" + stats = mock('Oxidized::Node::Stats') + stats.stubs(:successes).returns(1) + stats.stubs(:failures).returns(0) + stats.stubs(:get).returns(nil) + node = mock('Oxidized::Node') + node.stubs(:name).returns(malicious) + node.stubs(:stats).returns(stats) + @nodes.stubs(:each).yields(node) + + get '/nodes/stats' + + _(last_response.ok?).must_equal true + _(last_response.body).must_include(''><script>alert(1)</script>') + _(last_response.body).wont_include("'>") + _(last_response.body).wont_include('') + end + end + + describe 'config content encoding' do + it 'encodes script tags in device config on /node/version/view' do + payload = '' + @nodes.expects(:get_version).with('router1', '', 'abc123').returns(payload) + + get '/node/version/view?node=router1&group=&oid=abc123&epoch=0&num=1' + + _(last_response.ok?).must_equal true + _(last_response.body).must_include('<script>alert(1)</script>') + _(last_response.body).wont_include('') + end + + it 'encodes script tags in diff output on /node/version/diffs' do + payload_patch = "- \n+ safe line\n" + versions = [{ oid: 'C006', time: Time.parse('2025-02-05 19:49:00 +0100') }] + @nodes.expects(:version).with('router1', nil).returns(versions) + @nodes.expects(:get_diff).with('router1', '', 'C006', nil).returns( + { patch: payload_patch, stat: [1, 1] } + ) + + get '/node/version/diffs?node=router1&group=&oid=C006&epoch=0&num=1' + + _(last_response.ok?).must_equal true + _(last_response.body).must_include('<script>alert(1)</script>') + _(last_response.body).wont_include('') + end + end +end