Skip to content

Conversation

@bogdanandone
Copy link
Contributor

No description provided.

acinclude.m4 Outdated
Copy link
Member

Choose a reason for hiding this comment

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

why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the line forces libtool to compile both static and dynamic objects if sapi binaries are in the target;
without this line libtool compiles only PIC objects

@cjbj
Copy link
Contributor

cjbj commented Nov 25, 2015

Was this tested with --enable-dtrace?

@dstogov
Copy link
Member

dstogov commented Nov 26, 2015

I suppose no.

On Thu, Nov 26, 2015 at 12:46 AM, Christopher Jones <
notifications@github.com> wrote:

Was this tested with --enable-dtrace?


Reply to this email directly or view it on GitHub
#1650 (comment).

@bogdanandone
Copy link
Contributor Author

I did a build with --enable-dtrace configuration flag. I can confirm the followings:

  • Zend/.libs/zend_dtrace.d.o (the PIC object) is still there
  • Zend/zend_dtrace.d.o (the non PIC object) is generated (due to the patch)
  • Zend/zend_dtrace.d.lo (libtool helper object) has references to both PIC and non PIC object, as expected
  • php-cgi still works

Unfortunately I am unable to have a real dtrace run as I don't have an Oracle Linux platform.

@cjbj
Copy link
Contributor

cjbj commented Nov 26, 2015

Dtrace is available on OS X too, if that helps.

@dstogov
Copy link
Member

dstogov commented Nov 27, 2015

Why do you think, non-PIC build will prevent dtrace from work?
Also for "darwin" we already use non-PIC objects.
On Nov 27, 2015 2:04 AM, "Christopher Jones" notifications@github.com
wrote:

Dtrace is available on OS X too, if that helps.


Reply to this email directly or view it on GitHub
#1650 (comment).

@bogdanandone
Copy link
Contributor Author

OS X platform not available...
Theoretically speaking dtrace static or dynamic linkage should not be impacted as the patch is not replacing PIC objects with non PIC objects; dtrace PIC objects are still there; if dtrace needs Zend/.libs/zend_dtrace.d.o PIC object it will find it.
Even more, even if Zend/zend_dtrace.d.lo keeps reference to both objects, libtool will take into account by default the PIC version for linking; actually I didn't find a clean way to instruct libtool to link with static objects.

@laruence
Copy link
Member

doesn't "--with-pic Try to use only PIC/non-PIC objects default=use both" work?

@dstogov
Copy link
Member

dstogov commented Nov 30, 2015

It looks like libtool compiles each C file teice (with -fPIC and without),
but then use PIC objects anyway.

On Mon, Nov 30, 2015 at 9:13 AM, Xinchen Hui notifications@github.com
wrote:

doesn't "--with-pic Try to use only PIC/non-PIC objects default=use both"
work?


Reply to this email directly or view it on GitHub
#1650 (comment).

Cleaner than forcing 'php_sapi_module' as it simple preserves
enable_static=yes default value.
@remicollet
Copy link
Member

Notice: some linux distro requires to use PIC object for security reason.
E.g. https://fedoraproject.org/wiki/Packaging:Guidelines#PIE

@laruence
Copy link
Member

laruence commented Dec 8, 2015

then maybe we should make this can be controlled by configure option like --(en|dis)able-pic?

@weltling
Copy link
Contributor

weltling commented Dec 8, 2015

Thanks for the info, Remi.

Ping @oerdnj, what are the Debian policies on enforcing PIC?

@bogdanandone were you keen to study yet more the security implications? In Windows builds, /DYNAMICBASE is enabled always and cannot be disabled. IMHO this PR should be reevaluated from the security angle. The suggestion of @laruence to have a switch, probably that is disabled by default, could be an option.

Thanks.

@bogdanandone
Copy link
Contributor Author

Actually I was wondering why is PHP systematically using PIC objects everywhere ... Thanks to Remi now I have an answer :-) .

From your notes I understand that the capability to relocate binaries for security reasons should be preferred by default before performance.

I will look how --with-pic flag could be used. For now they have a strange behavior on my configuration so I have to go in a deeper investigation.

Thanks.

@oerdnj
Copy link
Contributor

oerdnj commented Dec 8, 2015

Not entirely sure that Debian has PIC as a system-wide policy, since Debian Hardening is still not release goal. But we definitely want have ASLR as much as possible for the code exposed to the wild wild internet. I think this would cause a lot of frowning in our security team and I would most probably revert this patch for Debian builds to have a PIC (and possibly PIE) build.

https://wiki.debian.org/Hardening

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants