Permalink
Browse files

Security: Fix many broken filter regexps

In Ruby, "foo\nbar" =~ /^bar/ will result in a match, because ^ matches
at the start of any line, not at the start of the string.  In general,
we want to use \A and \z in place of ^ and $, respectively.

We rely heavily on regular expressions to filter untrusted data.  And
many of these regular expressions can be fooled easily because they rely
on ^ and $ when they shouldn't.  See comment_drop_test for a
user-exploitable example.

This patch does a bulk search-and-replace of the offending patterns.  It
may easily have missed something somewhere, but it's a good start.
  • Loading branch information...
emk committed Dec 20, 2008
1 parent 173c3b8 commit c8c4bcc772301abcee6735466df93b60291878aa
@@ -26,15 +26,17 @@ X Can we restrict admin cookies to /admin ? No--need /accounts, too.
/ filtered_column_code_macro
/ filtered_column_flikr_macro - lots of issues
/ Do we have trackback support to check? No.
Password change
Verify token required to change e-mail and password
/ Password change
/ Verify token required to change e-mail and password
Everything else
/ Don't ship :session_key in environment.rb!
/ Do we need to override verifiable_request_format? No.
Check redirection in lib/authenticated_system.rb
/ Check redirection in lib/authenticated_system.rb
Detect mass assignment failures in unit tests
Review mass assignment in public controllers
Check regexes for ^ and $
/ Review mass assignment in public controllers - comments
/ Check regexes for ^ and $
Filter IMG tags
Block database updates on POST requests
Review http://guides.rubyonrails.org/security.html again

Admin only
@@ -138,7 +138,7 @@ def convert_times_to_utc
with_site_timezone do
date = Time.parse_from_attributes(params[:article], :published_at, :local)
next unless date
params[:article].delete_if { |k, v| k.to_s =~ /^#{:published_at}/ }
params[:article].delete_if { |k, v| k.to_s =~ /\A#{:published_at}/ }
params[:article][:published_at] = local_to_utc(date)
end
end
@@ -22,7 +22,7 @@ def allow_member?
end

def find_and_sort_templates
@layouts, @templates = site.templates.partition { |t| t.dirname.to_s =~ /layouts$/ }
@layouts, @templates = site.templates.partition { |t| t.dirname.to_s =~ /layouts\z/ }
end

def self.clear_empty_templates_for(model, *attributes)
@@ -7,7 +7,7 @@ def create
return
end

if params[:filename] =~ /\.(css|js)$/i
if params[:filename] =~ /\.(css|js)\z/i
@resource = @theme.resources.write params[:filename], params[:data]
redirect_to url_for_theme(:controller => 'resources', :action => 'edit', :filename => @resource.basename.to_s)
else
@@ -6,7 +6,7 @@ class FeedController < ApplicationController
def feed
sections = params[:sections].clone
last = sections.last
sections.delete(last) if last =~ /\.xml$/
sections.delete(last) if last =~ /\.xml\z/
@section_path = sections.blank? ? '' : sections.join('/')
case last
when 'all_comments.xml'
@@ -11,7 +11,7 @@ def initialize(source)

def author_url
return nil if source.author_url.blank?
@source.author_url =~ /^https?:\/\// ? @source.author_url : "http://" + @source.author_url
@source.author_url =~ /\Ahttps?:\/\// ? @source.author_url : "http://" + @source.author_url
end

def url
@@ -28,7 +28,7 @@ def textilize(text)

def parse_date(date)
return Time.now.utc if date.blank?
date = "#{date}-1" if date.to_s =~ /^\d{4}-\d{1,2}$/ unless [Time, Date].include?(date.class)
date = "#{date}-1" if date.to_s =~ /\A\d{4}-\d{1,2}\z/ unless [Time, Date].include?(date.class)
date = date.to_time
end

@@ -3,7 +3,7 @@ module ApplicationHelper

def author_link_for(comment)
return h(comment.author) if comment.author_url.blank?
link_to h(comment.author), "#{'http://' unless comment.author_url =~ /^https?:\/\//}#{comment.author_url}"
link_to h(comment.author), "#{'http://' unless comment.author_url =~ /\Ahttps?:\/\//}#{comment.author_url}"
end

def avatar_for(user, options = {})
@@ -14,11 +14,11 @@ class Asset < ActiveRecord::Base

class << self
def movie?(content_type)
content_type.to_s =~ /^video/ || extra_content_types[:movie].include?(content_type)
content_type.to_s =~ /\Avideo/ || extra_content_types[:movie].include?(content_type)
end

def audio?(content_type)
content_type.to_s =~ /^audio/ || extra_content_types[:audio].include?(content_type)
content_type.to_s =~ /\Aaudio/ || extra_content_types[:audio].include?(content_type)
end

def other?(content_type)
@@ -72,7 +72,7 @@ def full_filename(thumbnail = nil)

def public_filename_with_host(thumbnail = nil)
returning public_filename_without_host(thumbnail) do |s|
s.gsub! /^\/assets\/[^\/]+\//, "/assets/#{$1}" if Site.multi_sites_enabled
s.gsub! /\A\/assets\/[^\/]+\//, "/assets/#{$1}" if Site.multi_sites_enabled
end
end
alias_method_chain :public_filename, :host
@@ -20,8 +20,8 @@ def content_type(path)
def [](filename)
path =
case filename
when /\.js$/i then 'javascripts'
when /\.css$/i then 'stylesheets'
when /\.js\z/i then 'javascripts'
when /\.css\z/i then 'stylesheets'
else 'images'
end

@@ -260,8 +260,8 @@ def permalink_variable?(var)
end

def check_permalink_style
permalink_style.sub! /^\//, ''
permalink_style.sub! /\/$/, ''
permalink_style.sub! /\A\//, ''
permalink_style.sub! /\/\z/, ''
pieces = permalink_style.split('/')
errors.add :permalink_style, 'cannot have blank paths' if pieces.any?(&:blank?)
pieces.each do |p|
@@ -7,8 +7,8 @@ def template_types(extension = ".liquid")
end

def [](template_name)
template_name = File.basename(template_name.to_s).sub /#{theme.extension}$/, ''
theme.path + "#{template_name =~ /layout$/ ? 'layouts' : 'templates'}/#{template_name}#{theme.extension}"
template_name = File.basename(template_name.to_s).sub /#{theme.extension}\z/, ''
theme.path + "#{template_name =~ /layout\z/ ? 'layouts' : 'templates'}/#{template_name}#{theme.extension}"
end

def collect_templates(template_type, *custom_templates)
@@ -9,7 +9,7 @@ def self.import(zip_file, options = {})
dest = options[:to].is_a?(Pathname) ? options[:to] : Pathname.new(options[:to] || '.')
basename = dest.basename.to_s
if dest.exist? || basename == 'current'
basename = basename =~ /(.*)_(\d+)$/ ? $1 : basename
basename = basename =~ /(.*)_(\d+)\z/ ? $1 : basename
number = $2 ? $2.to_i + 1 : 2
dirname = dest.dirname
dest = dirname + "#{basename}_#{number}"
@@ -27,7 +27,7 @@ def self.import(zip_file, options = {})
dir_path = Pathname.new(dest + dir)
FileUtils.mkdir_p dir_path unless dir_path.exist?
z.dir.entries(dir).each do |entry|
next unless entry =~ /(\.\w+)$/ && allowed_extensions.include?($1)
next unless entry =~ /(\.\w+)\z/ && allowed_extensions.include?($1)
z.file.open(File.join(dir, entry)) { |zf| File.open(dir_path + entry, 'wb') { |f| f << zf.read } }
end
end
@@ -47,7 +47,7 @@ def initialize(base, site = nil)
@base_path = base
@path = Pathname.new(@base_path)
end
layout = (@path + "layouts").children(false).select {|v| v.to_s =~ /^layout/}[0] if (@path + "layouts").directory?
layout = (@path + "layouts").children(false).select {|v| v.to_s =~ /\Alayout/}[0] if (@path + "layouts").directory?
@extension = layout.extname if layout
end

@@ -1,6 +1,6 @@
module Format
# yes this is valid ruby, even if textmate's highlighter can't grok it
DOMAIN = /^([a-z0-9]([-a-z0-9]*[a-z0-9])?\.)+((a[cdefgilmnoqrstuwxz]|aero|arpa)|(b[abdefghijmnorstvwyz]|biz)|(c[acdfghiklmnorsuvxyz]|cat|com|coop)|d[ejkmoz]|(e[ceghrstu]|edu)|f[ijkmor]|(g[abdefghilmnpqrstuwy]|gov)|(h[kmnrtu]#{RAILS_ENV=='test'?'|host':''})|(i[delmnoqrst]|info|int)|(j[emop]|jobs)|k[eghimnprwyz]|l[abcikrstuvy]|(m[acdghklmnopqrstuvwxyz]|mil|mobi|museum)|(n[acefgilopruz]|name|net)|(om|org)|(p[aefghklmnrstwy]|pro)|qa|r[eouw]|s[abcdeghijklmnortvyz]|(t[cdfghjklmnoprtvwz]|travel)|u[agkmsyz]|v[aceginu]|w[fs]|y[etu]|z[amw])$/ unless const_defined?(:DOMAIN)
STRING = /^[a-z0-9-]+$/
DOMAIN = /\A([a-z0-9]([-a-z0-9]*[a-z0-9])?\.)+((a[cdefgilmnoqrstuwxz]|aero|arpa)|(b[abdefghijmnorstvwyz]|biz)|(c[acdfghiklmnorsuvxyz]|cat|com|coop)|d[ejkmoz]|(e[ceghrstu]|edu)|f[ijkmor]|(g[abdefghilmnpqrstuwy]|gov)|(h[kmnrtu]#{RAILS_ENV=='test'?'|host':''})|(i[delmnoqrst]|info|int)|(j[emop]|jobs)|k[eghimnprwyz]|l[abcikrstuvy]|(m[acdghklmnopqrstuvwxyz]|mil|mobi|museum)|(n[acefgilopruz]|name|net)|(om|org)|(p[aefghklmnrstwy]|pro)|qa|r[eouw]|s[abcdeghijklmnortvyz]|(t[cdfghjklmnoprtvwz]|travel)|u[agkmsyz]|v[aceginu]|w[fs]|y[etu]|z[amw])\z/ unless const_defined?(:DOMAIN)
STRING = /\A[a-z0-9-]+\z/
EMAIL = /(\A(\s*)\Z)|(\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\Z)/i
end
@@ -1,7 +1,7 @@
# Object model of a plugin in plugins/vendor. May or may not have an internal Mephisto::Plugins::Plugin (an AR).
module Mephisto
class DirectoryPlugin
@@filter = /^mephisto_(\w+)$/
@@filter = /\Amephisto_(\w+)\z/
attr_accessor :plugin, :path

def self.scan
@@ -1,7 +1,7 @@
module Mephisto
class Dispatcher
PERMALINK_OPTIONS = { :year => '\d{4}', :month => '\d{1,2}', :day => '\d{1,2}', :permalink => '[\w\-]+', :id => '\d+' }
PERMALINK_VAR = /^:([a-z]+)$/
PERMALINK_VAR = /\A:([a-z]+)\z/

def self.run(site, path)
# check for any bad urls like /foo//bar
@@ -80,7 +80,7 @@ def self.recognize_permalink(site, path)
result.first[var] = match[i+1]
end
result << match[variables.size + 2] # comments | comments.xml | changes.xml
result.last.gsub!(/\/(.*)$/, '') if result.last
result.last.gsub!(/\/(.*)\z/, '') if result.last
result << match[variables.size + 4] # comment id
end
end
@@ -29,15 +29,15 @@ def add_admin_tab(*args)
end

def mephisto_plugin?
name =~ /^mephisto_(\w+)$/
name =~ /\Amephisto_(\w+)\z/
end

def configurable?
not default_options.empty?
end

def mephisto_name
name.sub /^mephisto_/, ''
name.sub /\Amephisto_/, ''
end
alias :conf_name :mephisto_name
end
@@ -87,7 +87,7 @@ def self.handle_redirection(path)
end

protected
@@sanitize_path_regex = /^(\/)|(https?:\/\/)/
@@sanitize_path_regex = /\A(\/)|(https?:\/\/)/
def self.sanitize_path(path)
path =~ @@sanitize_path_regex ? path : "/#{path.split("://").last}"
end
@@ -2,7 +2,7 @@ module XMLRPC
module Convert
def self.dateTime(str)
case str
when /^(-?\d\d\d\d)-?(\d\d)-?(\d\d)T(\d\d):(\d\d):(\d\d)(?:Z|([+-])(\d\d):?(\d\d))?$/
when /\A(-?\d\d\d\d)-?(\d\d)-?(\d\d)T(\d\d):(\d\d):(\d\d)(?:Z|([+-])(\d\d):?(\d\d))?\z/
a = [$1, $2, $3, $4, $5, $6].collect{|i| i.to_i}
if $7
ofs = $8.to_i*3600 + $9.to_i*60
@@ -16,7 +16,7 @@ def self.dateTime(str)
a = [ utc.year, utc.month, utc.day, utc.hour, utc.min, utc.sec ]
end
XMLRPC::DateTime.new(*a)
when /^(-?\d\d)-?(\d\d)-?(\d\d)T(\d\d):(\d\d):(\d\d)(Z|([+-]\d\d):(\d\d))?$/
when /\A(-?\d\d)-?(\d\d)-?(\d\d)T(\d\d):(\d\d):(\d\d)(Z|([+-]\d\d):(\d\d))?\z/
a = [$1, $2, $3, $4, $5, $6].collect{|i| i.to_i}
if a[0] < 70
a[0] += 2000
@@ -40,6 +40,11 @@ def test_should_return_correct_author_link
assert_equal %Q{<a href="http://&lt;strong&gt;https://abc&lt;/strong&gt;">&lt;strong&gt;rico&lt;/strong&gt;</a>}, @comment.author_link
end

def test_should_not_be_fooled_by_newlines_in_author_url
@comment.source.author_url = "javascript:alert('Oops')\nhttp://"
assert_equal "http://javascript:alert('Oops')\nhttp://", @comment.author_url
end

def test_should_show_filtered_text
comment = contents(:welcome).comments.create :body => '*test* comment', :author => 'bob', :author_ip => '127.0.0.1'
assert_valid comment
@@ -74,4 +79,4 @@ def create_comment_stub(options)
def stub.id() 55; end
end
end
end
end

0 comments on commit c8c4bcc

Please sign in to comment.