这是indexloc提供的服务,不要输入任何密码
Skip to content
Closed
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
41 changes: 41 additions & 0 deletions src/etc/inc/certs.inc
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,42 @@ function ca_inter_create(& $ca, $keylen, $lifetime, $dn, $caref, $digest_alg = "
return true;
}

function ca_trustedcerts_generate() {
global $config, $g;
if (isset($config['system']['developerspew'])) {
$mt = microtime();
echo "ca_trustedcerts_generate() being called $mt\n";
}

safe_mkdir("{$g['etc_path']}/ssl/certs");
/* Remove all certs in the directory (but leave symlinks in place to allow power users to add trusted certs outside the Certificate Manager) */
array_map('unlink', array_filter(glob("{$g['etc_path']}/ssl/certs/*"),function($x) { return is_file($x) && !is_link($x); }));

if (is_array($config['ca'])) {
foreach ($config['ca'] as & $ca) {
if (isset($ca['trusted'])) {
$ca_hash = cert_get_hash($ca['crt']);
$ca_certfile = "{$g['etc_path']}/ssl/certs/{$ca_hash}.0";

/* Try again to unlink the specific cert file just in case a non-regular file (e.g. symlink) with the same hash exists */
unlink_if_exists($ca_certfile);
$fd = fopen($ca_certfile, "w");
if (!$fd) {
log_error(gettext(
"Error: cannot open {$g['etc_path']}/ssl/certs/{$ca_hash}.0 file in ca_trustedcerts_generate()."
));
return 1;
}

fwrite($fd, base64_decode($ca['crt']));
fclose($fd);
}
}
}

return 0;
}

function cert_import(& $cert, $crt_str, $key_str) {

$cert['crt'] = base64_encode($crt_str);
Expand Down Expand Up @@ -511,6 +547,11 @@ function cert_get_subject_hash($crt) {
return $inf_crt['subject'];
}

function cert_get_hash($crt) {
$inf_crt = openssl_x509_parse(base64_decode($crt));
return $inf_crt['hash'];
}

function cert_get_issuer($str_crt, $decode = true) {

if ($decode) {
Expand Down
62 changes: 26 additions & 36 deletions src/etc/inc/pfsense-utils.inc
Original file line number Diff line number Diff line change
Expand Up @@ -1834,24 +1834,20 @@ function get_freebsd_version() {
return $version[0];
}

function download_file($url, $destination, $verify_ssl = true, $connect_timeout = 5, $timeout = 0) {
function get_curl_handle($url, $connect_timeout = 5, $timeout = 0, $verify_ssl = true, $follow_location = true, $verbose = false) {
global $config, $g;

$fp = fopen($destination, "wb");

if (!$fp) {
return false;
}

$ch = curl_init();
curl_setopt($ch, CURLOPT_URL, $url);
curl_setopt($ch, CURLOPT_FOLLOWLOCATION, $follow_location);
curl_setopt($ch, CURLOPT_TIMEOUT, $timeout);
curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $connect_timeout);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, $verify_ssl);
curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, $verify_ssl);
curl_setopt($ch, CURLOPT_FILE, $fp);
curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $connect_timeout);
curl_setopt($ch, CURLOPT_TIMEOUT, $timeout);
curl_setopt($ch, CURLOPT_HEADER, false);
curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
curl_setopt($ch, CURLOPT_CAPATH, "{$g['etc_path']}/ssl/certs");
curl_setopt($ch, CURLOPT_VERBOSE, $verbose);

if (!isset($config['system']['do_not_send_host_uuid'])) {
curl_setopt($ch, CURLOPT_USERAGENT, $g['product_name'] . '/' . $g['product_version'] . ' : ' . get_single_sysctl('kern.hostuuid'));
} else {
Expand All @@ -1869,6 +1865,22 @@ function download_file($url, $destination, $verify_ssl = true, $connect_timeout
}
}

return $ch;
}

function download_file($url, $destination, $verify_ssl = true, $connect_timeout = 5, $timeout = 0) {
global $config, $g;

$fp = fopen($destination, "wb");

if (!$fp) {
return false;
}

$ch = get_curl_handle($url);
curl_setopt($ch, CURLOPT_FILE, $fp);
curl_setopt($ch, CURLOPT_HEADER, false);

@curl_exec($ch);
$http_code = curl_getinfo($ch, CURLINFO_HTTP_CODE);
fclose($fp);
Expand Down Expand Up @@ -1898,32 +1910,10 @@ function download_file_with_progress_bar($url, $destination, $verify_ssl = true,
* Originally by Author: Keyvan Minoukadeh
* Modified by Scott Ullrich to return Content-Length size
*/
$ch = curl_init();
curl_setopt($ch, CURLOPT_URL, $url);
curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, $verify_ssl);
curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, $verify_ssl);
$ch = get_curl_handle($url);
curl_setopt($ch, CURLOPT_HEADERFUNCTION, 'read_header');
curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
curl_setopt($ch, CURLOPT_WRITEFUNCTION, $readbody);
curl_setopt($ch, CURLOPT_NOPROGRESS, '1');
curl_setopt($ch, CURLOPT_CONNECTTIMEOUT, $connect_timeout);
curl_setopt($ch, CURLOPT_TIMEOUT, $timeout);
if (!isset($config['system']['do_not_send_host_uuid'])) {
curl_setopt($ch, CURLOPT_USERAGENT, $g['product_name'] . '/' . $g['product_version'] . ' : ' . get_single_sysctl('kern.hostuuid'));
} else {
curl_setopt($ch, CURLOPT_USERAGENT, $g['product_name'] . '/' . $g['product_version']);
}

if (!empty($config['system']['proxyurl'])) {
curl_setopt($ch, CURLOPT_PROXY, $config['system']['proxyurl']);
if (!empty($config['system']['proxyport'])) {
curl_setopt($ch, CURLOPT_PROXYPORT, $config['system']['proxyport']);
}
if (!empty($config['system']['proxyuser']) && !empty($config['system']['proxypass'])) {
@curl_setopt($ch, CURLOPT_PROXYAUTH, CURLAUTH_ANY | CURLAUTH_ANYSAFE);
curl_setopt($ch, CURLOPT_PROXYUSERPWD, "{$config['system']['proxyuser']}:{$config['system']['proxypass']}");
}
}
curl_setopt($ch, CURLOPT_NOPROGRESS, true);

@curl_exec($ch);
$http_code = curl_getinfo($ch, CURLINFO_HTTP_CODE);
Expand Down
8 changes: 3 additions & 5 deletions src/etc/inc/services.inc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
* limitations under the License.
*/

require_once('pfsense-utils.inc');

define('DYNDNS_PROVIDER_VALUES', 'all-inkl citynetwork cloudflare cloudflare-v6 custom custom-v6 dnsexit dnsimple dnsmadeeasy dnsomatic duiadns duiadns-v6 dyndns dyndns-custom dyndns-static dyns easydns eurodns freedns freedns-v6 glesys googledomains gratisdns he-net he-net-v6 he-net-tunnelbroker loopia namecheap noip noip-free ods opendns ovh-dynhost route53 selfhost spdyn spdyn-v6 zoneedit');
define('DYNDNS_PROVIDER_DESCRIPTIONS', 'All-Inkl.com,City Network,CloudFlare,CloudFlare (v6),Custom,Custom (v6),DNSexit,DNSimple,DNS Made Easy,DNS-O-Matic,DuiaDns.net,DuiaDns.net (v6),DynDNS (dynamic),DynDNS (custom),DynDNS (static),DyNS,easyDNS,Euro Dns,freeDNS,freeDNS (v6),GleSYS,Google Domains,GratisDNS,HE.net,HE.net (v6),HE.net Tunnelbroker,Loopia,Namecheap,No-IP,No-IP (free),ODS.org,OpenDNS,OVH DynHOST,Route 53,SelfHost,SPDYN,SPDYN (v6),ZoneEdit');

Expand Down Expand Up @@ -2032,12 +2034,8 @@ function dyndnsCheckIP($int) {
}

$hosttocheck = $url;
$ip_ch = curl_init($hosttocheck);
curl_setopt($ip_ch, CURLOPT_RETURNTRANSFER, 1);
curl_setopt($ip_ch, CURLOPT_SSL_VERIFYPEER, $verifysslpeer);
$ip_ch = get_curl_handle($hosttocheck, 30, 120, $verifysslpeer);
curl_setopt($ip_ch, CURLOPT_INTERFACE, 'host!' . $ip_address);
curl_setopt($ip_ch, CURLOPT_CONNECTTIMEOUT, '30');
curl_setopt($ip_ch, CURLOPT_TIMEOUT, 120);
curl_setopt($ip_ch, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4);
curl_setopt($ip_ch, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
curl_setopt($ip_ch, CURLOPT_USERPWD, "{$username}:{$password}");
Expand Down
6 changes: 6 additions & 0 deletions src/etc/pfSense-rc
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,12 @@ mkdir -p /usr/local/openssl >/dev/null 2>&1
ln -sf /etc/ssl/openssl.cnf \
/usr/local/openssl/openssl.cnf

# Add environment variable to custom CA trusted roots directory
# Use SSL_CA_CERT_PATH environment variable per man fetch(3)
export SSL_CA_CERT_PATH=/etc/ssl/certs
# Use SSL_CERT_DIR environment variable per man SSL_CTX_load_verify_locations(3)
export SSL_CERT_DIR=/etc/ssl/certs

# Run the php.ini setup file and populate
# /usr/local/etc/php.ini
/etc/rc.php_ini_setup 2>/tmp/php_errors.txt
Expand Down
5 changes: 5 additions & 0 deletions src/etc/rc.bootup
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ require_once("/etc/inc/filter.inc");
echo ".";
require_once("/etc/inc/shaper.inc");
echo ".";
require_once("/etc/inc/certs.inc");
echo ".";
require_once("/etc/inc/ipsec.inc");
echo ".";
require_once("/etc/inc/vpn.inc");
Expand Down Expand Up @@ -232,6 +234,9 @@ if (!$debugging) {
/* re-make hosts file after configuring interfaces */
system_hosts_generate();

/* add configured trusted CA certificates to /etc/ssl/certs */
ca_trustedcerts_generate();

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do trusted certs need to be re/generated during boot up? /etc/ssl/certs is a static dir is it not? The certs should still be there across reboots. Only reason I think of is restoring/swapping a config. But the trusted certs re/generation for that scenario should and could be done by other means. Such as part of the config restore process. Or a trusted certs regeneration button in ca manager for those times when manually swapping a config.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's safer to do it at boot time to ensure it's consistent. It's not like it's a huge amount of work or writes.

Copy link
Contributor

Choose a reason for hiding this comment

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

And completely unnecessary. It's not like people are restoring or swapping configs in prod every other time they reboot. Also not friendly to write cycle sensitive media.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you can check to be sure the state is consistent and update as needed, it is necessary. You don't know what someone might have done, or what might have happened on upgrade, etc. It's not like it's happening every few minutes, the number of writes overall is minimal. Better safe than sorry.

Copy link
Contributor

@NOYB NOYB Feb 20, 2017

Choose a reason for hiding this comment

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

Unless you can check to be sure the state is consistent and update as needed, it is necessary.

Which is how it should be done. Verify what is there against what config says should be there and add/remove accordingly. Also include a "synchronize" button on the CA manager page for user to be able to manually initiated the same thing.

/* start OpenVPN server & clients */
echo "Syncing OpenVPN settings...";
openvpn_resync_all();
Expand Down
8 changes: 2 additions & 6 deletions src/usr/local/www/crash_reporter.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,9 @@ function upload_crash_report($files) {
$post["file{$counter}"] = "@{$file}";
$counter++;
}
$ch = curl_init();
curl_setopt($ch, CURLOPT_HEADER, 0);
curl_setopt($ch, CURLOPT_VERBOSE, 0);
$ch = get_curl_handle($g['crashreporterurl']);
curl_setopt($ch, CURLOPT_HEADER, false);
curl_setopt($ch, CURLOPT_SAFE_UPLOAD, false);
curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_USERAGENT, $g['product_name'] . '/' . $g['product_version']);
curl_setopt($ch, CURLOPT_URL, $g['crashreporterurl']);
curl_setopt($ch, CURLOPT_POST, true);
curl_setopt($ch, CURLOPT_POSTFIELDS, $post);
$response = curl_exec($ch);
Expand Down
14 changes: 14 additions & 0 deletions src/usr/local/www/system_camanager.php
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@
$name = $a_ca[$id]['descr'];
unset($a_ca[$id]);
write_config();
ca_trustedcerts_generate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would rather trusted certs be generated/removed on a per cert add/edit basis instead of regenerating all of them every time a change is made. Even changes that do not make changes to trusted certs settings.

$savemsg = sprintf(gettext("Certificate Authority %s and its CRLs (if any) successfully deleted."), htmlspecialchars($name));
pfSenseHeader("system_camanager.php");
exit;
Expand All @@ -107,6 +108,7 @@
if (!empty($a_ca[$id]['prv'])) {
$pconfig['key'] = base64_decode($a_ca[$id]['prv']);
}
$pconfig['trusted'] = isset($a_ca[$id]['trusted']);
}

if ($act == "new") {
Expand Down Expand Up @@ -250,6 +252,8 @@

$ca['descr'] = $pconfig['descr'];

$ca['trusted'] = isset($pconfig['trusted']);

if ($act == "edit") {
$ca['descr'] = $pconfig['descr'];
$ca['refid'] = $pconfig['refid'];
Expand Down Expand Up @@ -312,6 +316,7 @@

if (!$input_errors) {
write_config();
ca_trustedcerts_generate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would rather trusted certs be generated/removed on a per cert add/edit basis instead of regenerating all of them every time a change is made. Even changes that do not make changes to trusted certs settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure it is a good idea to trust that files named *hashvalue*.0 have the correct certificate as their contents. I could certainly read in the existing file and check if it contains the correct certificate, but that seems like a lot of overhead compared to rewriting the certfiles during change/reboot. Thoughts on security implications of blindly trusting the file contents?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are some more comments on this up higher. I'd rather see them rewritten each boot to be absolutely sure. I don't see saving 2-3 writes per reboot as being worth all the extra complexity. You could read the files in and compare, or compute and compare hashes without reading directly, but it's a lot of overhead for minimal benefit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Deleting the entire set of cert files and rewriting them provides no additional safety.

  1. If the contained cert does not match the hash name then it will fail for the site. The certs here are found for the site by their hash name. They are not all read until a match is found. That's why they are required to be hash named.

  2. Symlinks are not being destroyed. So anyone who wishes to tamper can do so simply by creating a link to a cert located anywhere on the system they want.

  3. Anyone wishing to tamper can simply put a cert of their choosing in the CA bundle (/usr/local/share/certs/ca-root-nss.crt). It is not purged and reloaded at boot either.

This argument of safety and trust is nonsensical.

No need to read the files in and compare. Just create an array of file names config says should be there and add/remove accordingly. Not much work at all really, and far less work than writing files that don't need to be written. Deleting and rewriting the files is much more work for no benefit.

// Get list of trust files for CA certs from config.
foreach($config['ca'] as $ca) {
	$ca_hash = cert_get_hash($ca['crt']);
	$ca_certfiles[] = "{$g['etc_path']}/ssl/certs/{$ca_hash}.0";
}

// Remove cert trust files that have no corresponding CA cert in config.
foreach (glob("{$g['etc_path']}/ssl/certs/*") as $cert_file) {
	if (!in_array($cert_file, $ca_certfiles)) {
		unlink_if_exists($cert_file);
	}
}

// Add missing cert trust files for CA certs in config.  Remove cert trust files of CA certs in config that are not trusted or are revoked.
foreach ($a_ca as $id => $value) {
	ca_trustedcert_add($id);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Wanting to be sure the firewall has correct contents in the files is hardly "nonsensical". When I say "safe" I mean "Ensure the contents of the files are correct and that all are present", not necessarily "safe" from a security standpoint (which is also important).

If a file is named properly but contains the wrong contents (0-byte file, truncated, overwritten, etc) it would be completely missed unless the contents of the files are checked/verified. There would be no way to correct that unless the file is overwritten or removed and rewritten. The use of the CA would fail but there's no way to correct that, and it wouldn't be obvious to the user that they might have to trigger a forced rewrite by edit/save of the CA if it was fixed to force an update then.

You're still jumping through a lot of hoops to avoid something that isn't even a sizable problem. A few writes per reboot isn't going to kill a disk. We need to be sure the files are in a consistent state with correct contents, making no assumptions and taking no shortcuts.

Copy link
Contributor

@NOYB NOYB Feb 21, 2017

Choose a reason for hiding this comment

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

Just because it's the way you what pfSense to behave does not make it good or even beneficial. Microsoft does things because that's they way they want it to be too. Doesn't necessarily make it better. Doing that way just because that's the way we want it to be is not sound design. Any good engineer will tell you that the more you "touch" something increases the probability of introducing an error/failure/etc. The file is there, it's good. Leave it alone and let it be. Just the like CA bundle file.

If customers are willing to accept your inconsistently applied bloatware method of file integrity assurance. That is between them and you. But yes I prefer it be left alone once there. And that's how it will be done on my systems.

If it really is that critically important to you/pfSense then the rewriting on boot up is still not the "right"/"best" way of ensuring the content is correct, and has much more overhead than doing it in a well engineered fashion that could also be applied to any file deemed to be critical.

Copy link
Contributor

Choose a reason for hiding this comment

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

You disagree, I get it. Let it go. No need for insults or name-calling. Your way is not better, it's riskier. You don't know the file is good until you check it to know it's good. You want to allow for the possibility that it's not right -- that's on you, but that isn't how we operate. Overwrite or read/verify checksum, either way is fine since you end up with a file in a known-good state.

Leaving unverified data in place because the filename happens to be correct isn't "well engineered".

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. No need for insults or name-calling.

Where did I insult you? Where did I call you a name?

You certainly hurled some insults at me though.

My way is no more riskier than how the CA bundle is currently being handled. And like I stated. Rewriting files at boot up is still not the "right" way to ensure file integrity. There are better, more reliable methods with far less overhead.

Leaving unverified data in place because the filename happens to be correct isn't "well engineered".

But you and pfSense are okay with that as long is it's other just as critical files. Yes. That logic is well reasoned out. Not.

I have never said they shouldn't be verified. But if they are going to be then do it "right". With a well engineer method and consistently apply it to critical files.

Copy link
Contributor

Choose a reason for hiding this comment

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

"inconsistently applied bloatware", for example. Uncalled for. You keep arguing in circles ignoring my explanations, so I'm done.

Copy link
Contributor

Choose a reason for hiding this comment

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

Talking down to someone with statements like; let me boil it down to basic, so you can get it and such. Yes those kinds of statements that you made are insults. I wasn't born yesterday. I know what those statements are about. They are intended to belittle.

I'm not the one arguing in circles and ignoring your explanations. I'm debunking them and the reasoning that's being given for them.

pfSenseHeader("system_camanager.php");
}
}
Expand Down Expand Up @@ -362,6 +367,7 @@
<tr>
<th><?=gettext("Name")?></th>
<th><?=gettext("Internal")?></th>
<th><?=gettext("Trusted")?></th>
<th><?=gettext("Issuer")?></th>
<th><?=gettext("Certificates")?></th>
<th><?=gettext("Distinguished Name")?></th>
Expand Down Expand Up @@ -405,6 +411,7 @@
<tr>
<td><?=$name?></td>
<td><i class="fa fa-<?= (!empty($ca['prv'])) ? "check" : "times" ; ?>"></i></td>
<td><i class="fa fa-<?= (isset($ca['trusted'])) ? "check" : "times" ; ?>"></i></td>
<td><i><?=$issuer_name?></i></td>
<td><?=$certcount?></td>
<td>
Expand Down Expand Up @@ -486,6 +493,13 @@
$pconfig['descr']
));

$section->addInput(new Form_Checkbox(
'trusted',
'Trust',
'Trust this CA\'s certificates',
$pconfig['trusted']
))->setHelp('Trust certificates generated by this CA when pfSense accesses HTTPS/FTPS servers (e.g. updates, firewall URL aliases).');

if (!isset($id) || $act == "edit") {
$section->addInput(new Form_Select(
'method',
Expand Down